rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528617884
##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -495,6 +495,10 @@ public RexNode field(int inputCount, String alias, String
fieldName) {
/** Returns a reference to a given field of a record-valued expression. */
public RexNode field(RexNode e, String name) {
+ if (!e.getType().isStruct()) {
+ throw new IllegalArgumentException("Requested field " + name + " in
non-struct expression "
+ + e.toString() + " (type: " + e.getType() + ")");
+ }
Review comment:
The reason would be having the same "tailored check" in both RexBuilder
methods:
```
public RexNode makeFieldAccess(RexNode expr, String fieldName, boolean
caseSensitive) {...}
public RexNode makeFieldAccess(RexNode expr,int i) {...}
```
Also, there is the fact that assertions can be deactivated, so I would lean
towards an `IllegalArgumentException` in this case.
Finally, we do know that `RelDataTypeImpl` has some assertions in place, but
it is not defined the behavior that other `RelDataType` implementions may have
in this situation.
Having said that, if the community thinks these checks are redundant, I am
not against removing them, keeping both `RexBuilder#makeFieldAccess` methods as
they are; and in case of incorrect calls they will both eventually fail with
`AssertionError` thrown by `RelDataTypeImpl` when the try to call `getField` or
`getFieldList`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
