[GitHub] [calcite] julianhyde commented on a diff in pull request #3167: [CALCITE-5660] Implement BigQuery Array Subscript Operators

["julianhyde (via GitHub)" <gi...@apache.org>]

julianhyde commented on code in PR #3167:
URL: https://github.com/apache/calcite/pull/3167#discussion_r1173179787


##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -678,7 +682,13 @@ Builder populate2() {
       final RexCallImplementor value = new ValueConstructorImplementor();
       map.put(MAP_VALUE_CONSTRUCTOR, value);
       map.put(ARRAY_VALUE_CONSTRUCTOR, value);
-      map.put(ITEM, new ItemImplementor());
+
+      // Each subscript operator has a corresponding offset and 'safe' value.

Review Comment:
   Better to move `offset` and `safe` from `ItemImplementor` into `SqlItemOperator` (make them public final fields)
   
   Right now you're leaving a little too heavily on object identity (and there's a danger that future maintainers will branch based on the name of the operator, which will be bad).



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3372,11 +3372,16 @@ public static String replace(String s, String search, String replacement) {
 
   /** Helper for "array element reference". Caller has already ensured that
    * array and index are not null. Index is 1-based, per SQL. */
-  public static @Nullable Object arrayItem(List list, int item) {
-    if (item < 1 || item > list.size()) {
-      return null;
+  public static @Nullable Object arrayItem(List list, int item, int offset, boolean safe) {
+

Review Comment:
   remove the blank line



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org