julianhyde commented on code in PR #3167: URL: https://github.com/apache/calcite/pull/3167#discussion_r1187827849
########## core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java: ########## @@ -3090,6 +3102,13 @@ private static class ItemImplementor extends AbstractRexCallImplementor { final RexCall call, final List<Expression> argValueList) { final MethodImplementor implementor = getImplementor(call.getOperands().get(0).getType().getSqlTypeName()); + // If the structure is an array, two additional arguments are added to the argValueList + // to denote what the offset is and what the behavior should be if the index is out of boudns. + if (implementor.method.getName().equals("arrayItemOptional")) { Review Comment: Hard-coded method names are too fragile. Especially strange given that getImplementor returns BuiltInMethod.ARRAY_ITEM and there's no BuiltInMethod.ARRAY_ITEM_OPTIONAL. Reading this code, I first thought it was because of the 'optional'. But actually it's because it's an array. So, figure out a way to make this code more robust and intuitive. It's not sufficient that it works. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org