do we need to add something similar for the other operators

safe_offset, safe_ordinal etc

In fact, Calcite currently treats a simple array index as a SAFE_ORDINAL index, as the comment on SqlOperator ITEM says:

   /** The SQL standard calls the ARRAY variant a
   * <array element reference>. Index is 1-based. The standard says
   * to raise "data exception - array element error" but we currently return
   * null. */
   public static final SqlOperator ITEM =
      new SqlItemOperator("ITEM", OperandTypes.ARRAY_OR_MAP, 1, true);

The last true argument is for the safe parameter.

If we take this approach, a raw index is unparsed as a SAFE_ORDINAL, reflecting the implementation semantics.
If you want to preserve the original syntax we need to carry additional information in the SqlItemOperator.
What is the right solution?

In other words, should the following test pass?

@Test void testOffset() {
    sql("SELECT ARRAY[2,4,6][2]")
        .ok("SELECT (ARRAY[2, 4, 6])[SAFE_ORDINAL(2)]");
    sql("SELECT ARRAY[2,4,6][OFFSET(2)]")
        .ok("SELECT (ARRAY[2, 4, 6])[OFFSET(2)]");
    sql("SELECT ARRAY[2,4,6][SAFE_OFFSET(2)]")
        .ok("SELECT (ARRAY[2, 4, 6])[SAFE_OFFSET(2)]");
    sql("SELECT ARRAY[2,4,6][SAFE_ORDINAL(2)]")
        .ok("SELECT (ARRAY[2, 4, 6])[SAFE_ORDINAL(2)]");
  }

I would imagine you do not want to unparse with SAFE_ORDINAL unless you are using BigQuery as it is dialect-specific syntax (to my knowledge). The comment you mentioned- it seems like a good time to address that as well if possible.

The BigQuerySqlDialect already handles this correctly.
The bug I am fixing was caused by SqlOperatorTest, where tests look like this:

final SqlOperatorFixture f0 = fixture();
    f0.setFor(SqlLibraryOperators.SAFE_ORDINAL);
    final SqlOperatorFixture f = f0.withLibrary(SqlLibrary.BIG_QUERY);
    f.checkScalar("ARRAY[2,4,6][SAFE_ORDINAL(3)]", "6", "INTEGER");    

Notice that this test only sets up the library, but not the dialect. In a new test fixture that I am writing, which unparses and reparses the query before executing, this test fails because the query is not unparsed correctly. In this case the unparsing does not go through the dialect.

I don't see a way to set the dialect in the SqlOperatorFixture. Do you have a suggestion?

Moreover, the SQL parser will happily parse the SAFE_* functions without the big query dialect.

I know you can create the fixture with a specified parser config (see withParserConfig()) which I believe certain dialects like BigQuery have default parser configs. Perhaps you could pull on this thread a bit, the functionality may already be there.

True, but I still need to unparse such a query when the dialect is NOT specified.
The parser accepts SAFE_ORDINAL, so the unparser should too.

Ah I see what you're saying. Yeah I believe when I originally added this cluster of functions that I essentially pair the OFFSET operator with ITEM because at the time they seemed identical. Admittedly I did not give enough thought to the unparsing so maybe treating them more independently would help also, given that that approach seems to work for most other BQ-specific operators.

I think String funcId is more valuable than boolean simple.

If we have String funcId, we can write the code

if (funcId != null) {
      final SqlWriter.Frame offsetFrame = writer.startFunCall(funcId);
      call.operand(1).unparse(writer, 0, 0);
      writer.endFunCall(offsetFrame);
}
else {
      call.operand(1).unparse(writer, 0, 0);
}

This is much better, thank you.

IMO, a tag should contain Jira summary.