You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/11/19 16:05:56 UTC
[GitHub] [calcite] rubenada opened a new pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
rubenada opened a new pull request #2272:
URL: https://github.com/apache/calcite/pull/2272
Jira: [CALCITE-4409](https://issues.apache.org/jira/browse/CALCITE-4409)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada merged pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2272:
URL: https://github.com/apache/calcite/pull/2272
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528563712
##########
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:
I believe this check should be moved to `org.apache.calcite.rel.type.RelDataTypeImpl#getField`
Then all uses of `RelDataTypeImpl#getField` would get the proper exception rather than NPE.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#issuecomment-733538377
@vlsi @zabetak @danny0405 are we satisfied with the current status of the PR? Any other remarks? Shall I go on and squash commits and then merge?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528565876
##########
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:
@vlsi we had the same thought :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530277285
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
Exception message reviewed to make it more high level
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528622265
##########
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:
I'm inclined it should be `IllegalStateException` since any argument to `RelDataTypeImpl#getField` would result in an exception.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530843016
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -2577,6 +2577,22 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(ex.getMessage(), allOf(containsString("Expression"), containsString("not found")));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4409">[CALCITE-4409]
+ * Improve exception when RelBuilder tries to create a field on a non-struct expression</a>. */
+ @Test void testFieldOnNonStructExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ IllegalStateException ex = assertThrows(IllegalStateException.class, () -> {
+ builder.scan("EMP")
+ .project(
+ builder.field(builder.field("EMPNO"), "abc"))
+ .build();
+ }, "Field should fail since we are trying access a field on expression with non-struct type");
+ assertThat(ex.getMessage(),
+ allOf(containsString("Trying to access field"),
+ containsString("in a type with no fields")));
Review comment:
Just to confirm, the verification with `is(...)` would look like:
`assertThat(ex.getMessage(), is("Trying to access field abc in a type with no fields: SMALLINT"));`
Is that what you both would prefer?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528631088
##########
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:
Ah, indeed, `RelCrossType` is a case when `isStruct == false` and `getFieldList()` returns non-null, so `.isStruct()` check must not be added to `RexBuilder`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528690456
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
In which case, use can pass in a null fieldList ? I would rather to move the check to the builder that generate the `RelDataTypeImpl` instance.
The exception message is also confusing, we should tell user why he does wrong but not "we got null field list".
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528596915
##########
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:
After a second thought, apart from changing RelDataTypeImpl to have an AssertionError instead of NPE, I added a specific check in RexBuilder, in order to have the same behavior in both public methods `makeFieldAccess`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530854802
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -2577,6 +2577,22 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(ex.getMessage(), allOf(containsString("Expression"), containsString("not found")));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4409">[CALCITE-4409]
+ * Improve exception when RelBuilder tries to create a field on a non-struct expression</a>. */
+ @Test void testFieldOnNonStructExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ IllegalStateException ex = assertThrows(IllegalStateException.class, () -> {
+ builder.scan("EMP")
+ .project(
+ builder.field(builder.field("EMPNO"), "abc"))
+ .build();
+ }, "Field should fail since we are trying access a field on expression with non-struct type");
+ assertThat(ex.getMessage(),
+ allOf(containsString("Trying to access field"),
+ containsString("in a type with no fields")));
Review comment:
Ok, changed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528628858
##########
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:
Basically, I do not like to make `RexBuilder` aware of the ways `getField` can fail.
In other words, it is not RexBuilder's business to tell if `non-structs` are allowed to have fields or not.
I've had a very similar discussion with @julianhyde in https://issues.apache.org/jira/browse/CALCITE-4217, and the takeaway was "don't try to make sense of struct vs fieldList".
That is why I believe the necessary and sufficient solution here is:
1) Delegate the proper exception message to `org.apache.calcite.rel.type.RelDataType#getField(String fieldName, ...)` implementations. In other words, if someone implements `RelDataType`, then it is their business to throw the appropriate exception.
2) `org.apache.calcite.rex.RexBuilder#makeFieldAccess(org.apache.calcite.rex.RexNode, int)` is slightly different since it can't delegate to `RelDataType`, and it has to call `type.getFieldList()`. It could be left as is, and the type implementation would throw "type ... has no fields" (which might be enough).
If you want to add clarification with the problematic `RexNode`, then you could use
```java
try {
final List<RelDataTypeField> fields = type.getFieldList();
} catch (RuntimeException e) {
e.addSuppressed(new Throwable("Unable to get field " + i + " from rexNode " + rexNode");
throw e
}
```
The important point is in both cases `RexBuilder` makes no assumptions on `struct` vs `non-struct`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530460652
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
I’d prefer a simpler error message, such as “Type does not support fields”. Especially if it’s emitted by RelDataType, which is a low level class and not supposed to be user-friendly.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528690456
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
In which case, user can pass in a null fieldList ? I would rather to move the check to the builder that generate the `RelDataTypeImpl` instance.
The exception message is also confusing, we should tell user why he does wrong but not "we got null field list".
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528544841
##########
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:
I am wondering where is it better to add this check here or inside `RexBuilder#makeFieldAccess`. Clients who call `RexBuilder` direclty will still get the NPE.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530479592
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
Please suggest exact pattern to insert into `new IllegalStateException(...)`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530183290
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
I agree that `type with null fieldList` is too low-level for `RelDataType` users, however, I don't know the better message.
The following might be slightly bit better: `"The current type " + this + " does not support named field lookup, so field " + fieldName + " can't be located"`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#issuecomment-733805803
Let's not make a big fuss out of an exception message. I have committed a new proposal. I think it is simple, meaningful, not too low level.
If someone has an objection please let me know, otherwise I plan to merge this (small) patch in the next 24h.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528631088
##########
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:
Ah, indeed, `RelCrossType` is a case when `isStruct == false` and `getFieldList()` returns non-null.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528634272
##########
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:
> Ah, indeed, RelCrossType is a case when isStruct == false and getFieldList() returns non-null.
Well spotted, thanks for the feedback. I'll revert the changes in RexBuilder
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530843569
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -2577,6 +2577,22 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(ex.getMessage(), allOf(containsString("Expression"), containsString("not found")));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4409">[CALCITE-4409]
+ * Improve exception when RelBuilder tries to create a field on a non-struct expression</a>. */
+ @Test void testFieldOnNonStructExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ IllegalStateException ex = assertThrows(IllegalStateException.class, () -> {
+ builder.scan("EMP")
+ .project(
+ builder.field(builder.field("EMPNO"), "abc"))
+ .build();
+ }, "Field should fail since we are trying access a field on expression with non-struct type");
+ assertThat(ex.getMessage(),
+ allOf(containsString("Trying to access field"),
+ containsString("in a type with no fields")));
Review comment:
Exactly.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530836466
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -2577,6 +2577,22 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(ex.getMessage(), allOf(containsString("Expression"), containsString("not found")));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4409">[CALCITE-4409]
+ * Improve exception when RelBuilder tries to create a field on a non-struct expression</a>. */
+ @Test void testFieldOnNonStructExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ IllegalStateException ex = assertThrows(IllegalStateException.class, () -> {
+ builder.scan("EMP")
+ .project(
+ builder.field(builder.field("EMPNO"), "abc"))
+ .build();
+ }, "Field should fail since we are trying access a field on expression with non-struct type");
+ assertThat(ex.getMessage(),
+ allOf(containsString("Trying to access field"),
+ containsString("in a type with no fields")));
Review comment:
Simply because the actual fieldName value goes in between, and it is not really relevant in order to verify the exception message. I just followed the same pattern as the test just above.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530473402
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
My suggestion is meaningful, too.
The verbose message says that the type doesn’t support named field lookup. So the reader will wonder whether it supports field lookup by other means. No, it doesn’t have fields. Just say that.
A simple message is sufficient for the user to diagnose their problem.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530463898
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
Exceptions should always be meaningful, even the low-level ones.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528606836
##########
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:
An assertion in `RelDataTypeImpl` is enough. What is the purpose of duplicating the same check in multiple places?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528559816
##########
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:
I have the impression that the `RelBuilder` build rel trees based on all kinds of assumptions, it assumes that the invoker knows how to behave with the valid operands. We may need to check all the interfaces in `RelBuilder`, this PR is a good start.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530840840
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -2577,6 +2577,22 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(ex.getMessage(), allOf(containsString("Expression"), containsString("not found")));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4409">[CALCITE-4409]
+ * Improve exception when RelBuilder tries to create a field on a non-struct expression</a>. */
+ @Test void testFieldOnNonStructExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ IllegalStateException ex = assertThrows(IllegalStateException.class, () -> {
+ builder.scan("EMP")
+ .project(
+ builder.field(builder.field("EMPNO"), "abc"))
+ .build();
+ }, "Field should fail since we are trying access a field on expression with non-struct type");
+ assertThat(ex.getMessage(),
+ allOf(containsString("Trying to access field"),
+ containsString("in a type with no fields")));
Review comment:
The field name does not change over time, so it would be better to use regular `is("....")` matcher to get better feedback in IDE and CI
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528563037
##########
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:
Thanks for the feedback. Looking again at this code in particular, I have the impression that a simpler fix for this particular issue would be leaving `RelBuilder` untouched and modify this line in `RelDataTypeImpl.getField`:
```
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
for (RelDataTypeField field : fieldList) {
// =>
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
for (RelDataTypeField field : getFieldList()) { // change here
```
With this change, the scenario in the Jira would fail with an AssertionError (thrown by `getFieldList()`) instead of NPE.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528627634
##########
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:
I proposed `IllegalArgumentException`, considering the first argument to be the illegal one (`RexNode expr`, having a non-struct type). But I can see `IllegalStateException` fitting here as well.
@zabetak @danny0405 what are your thoughts regarding the new proposal? Are you against adding the check in `RexBuilder#makefieldAccess` (hence having an `AssertionError` from `RelDataTypleImpl`); or for adding check (in which case, would you see it as an `IllegalArgumentException` or `IllegalStateException`)?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528621824
##########
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:
@vlsi if I am not mistaken, `org.apache.calcite.piglet.DynamicTupleRecordType` inherits `isStruct` from `org.apache.calcite.rel.type.DynamicRecordTypeImpl`:
```
@Override public boolean isStruct() {
return true;
}
```
But, what you say is a valid point, if we have the assumption that a non-struct type can support getField, then `isStruct()` check must not be added. I'm not sure if this scenario makes sense at all, but just in case perhaps the most conservative approach would be removing the `isStruct()` check and just rely on the `AssertionError` thrown by `RelDataTypeImpl`.
Honestly, this is a minor change, I am open to any suggestion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#issuecomment-733791196
@rubenada I think the message should be simple and terse. Less is more.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] danny0405 commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r530732826
##########
File path: core/src/test/java/org/apache/calcite/test/RelBuilderTest.java
##########
@@ -2577,6 +2577,22 @@ private RelNode buildRelWithDuplicateAggregates(
assertThat(ex.getMessage(), allOf(containsString("Expression"), containsString("not found")));
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4409">[CALCITE-4409]
+ * Improve exception when RelBuilder tries to create a field on a non-struct expression</a>. */
+ @Test void testFieldOnNonStructExpression() {
+ final RelBuilder builder = RelBuilder.create(config().build());
+ IllegalStateException ex = assertThrows(IllegalStateException.class, () -> {
+ builder.scan("EMP")
+ .project(
+ builder.field(builder.field("EMPNO"), "abc"))
+ .build();
+ }, "Field should fail since we are trying access a field on expression with non-struct type");
+ assertThat(ex.getMessage(),
+ allOf(containsString("Trying to access field"),
+ containsString("in a type with no fields")));
Review comment:
What is the purpose to split into two `containsString` here ?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#issuecomment-734242901
Merged.
Thanks everyone for the feedback.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528563037
##########
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:
Thanks for the feedback. Looking again at this code in particular, I have the impression that a simpler fix for this specific issue would be leaving `RelBuilder` untouched and modify this line in `RelDataTypeImpl.getField`:
```
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
for (RelDataTypeField field : fieldList) {
// =>
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
for (RelDataTypeField field : getFieldList()) { // change here
```
With this change, the scenario in the Jira would fail with an AssertionError (thrown by `getFieldList()`) instead of NPE.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r529270619
##########
File path: core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
##########
@@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
@Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
boolean elideRecord) {
+ if (fieldList == null) {
+ throw new IllegalStateException("Requested field " + fieldName
+ + " in a type with null fieldList, type = " + this);
Review comment:
@danny0405 any "simple" (i.e. non-struct) type will have a null fieldList.
The problem is not having a null fieldList (which is valid), the problem is trying to access this list (e.g. via getField, but also getFieldList, getFieldNames, getFieldCount...) when this list is null
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
julianhyde commented on pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#issuecomment-733796356
No. You seem to have made a new rule “anyone who critiques a PR has to
write code to fix it”. That’s not how it works.
On Wed, Nov 25, 2020 at 7:58 AM Vladimir Sitnikov <no...@github.com>
wrote:
> *@vlsi* commented on this pull request.
> ------------------------------
>
> In core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java
> <https://github.com/apache/calcite/pull/2272#discussion_r530479592>:
>
> > @@ -79,6 +79,10 @@ protected RelDataTypeImpl() {
>
> @Override public RelDataTypeField getField(String fieldName, boolean caseSensitive,
> boolean elideRecord) {
> + if (fieldList == null) {
> + throw new IllegalStateException("Requested field " + fieldName
> + + " in a type with null fieldList, type = " + this);
>
> Please suggest exact pattern to insert into new IllegalStateException(...)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/calcite/pull/2272#discussion_r530479592>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAIUAOIUJYNEZQ3R736LZC3SRUSRBANCNFSM4T3TA5EQ>
> .
>
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528628858
##########
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:
Basically, I do not like to make `RexBuilder` aware of the ways `getField` can fail.
In other words, it is not RexBuilder's business to tell if `non-sturcts` are allowed to have fields or not.
I've had a very similar discussion with @julianhyde in https://issues.apache.org/jira/browse/CALCITE-4217, and the takeaway was "don't try to make sense of struct vs fieldList".
That is why I believe the necessary and sufficient solution here is:
1) Delegate the proper exception message to `org.apache.calcite.rel.type.RelDataType#getField(String fieldName, ...)` implementations. In other words, if someone implements `RelDataType`, then it is their business to throw the appropriate exception.
2) `org.apache.calcite.rex.RexBuilder#makeFieldAccess(org.apache.calcite.rex.RexNode, int)` is slightly different since it can't delegate to `RelDataType`, and it has to call `type.getFieldList()`. It could be left as is, and the type implementation would throw "type ... has no fields" (which might be enough).
If you want to add clarification with the problematic `RexNode`, then you could use
```java
try {
final List<RelDataTypeField> fields = type.getFieldList();
} catch (RuntimeException e) {
e.addSuppressed(new Throwable("Unable to get field " + i + " from rexNode " + rexNode");
throw e
}
```
The important point is in both cases `RexBuilder` makes no assumptions on `struct` vs `non-struct`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528634272
##########
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:
> Ah, indeed, RelCrossType is a case when isStruct == false and getFieldList() returns non-null.
Well spotted, thanks for the feedback. I'll revert the changes in RexBuilder
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528566370
##########
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:
>With this change, the scenario in the Jira would fail with an AssertionError (thrown by getFieldList()) instead of NPE.
Please use the tailored null check and the proper exception so the user could know which field is wrong. Then user could relate the problematic field name to the input SQL.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528621824
##########
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:
@vlsi if I am not mistaken, `org.apache.calcite.piglet.DynamicTupleRecordType` inherits `isStruct` from `org.apache.calcite.rel.type.DynamicRecordTypeImpl`:
```
@Override public boolean isStruct() {
return true;
}
```
But, what you say is a valid point, if we have the assumption that a non-struct type can support getField, then `isStruct()` check must not be added. I'm not sure if this scenario makes sense at all (it seems to go against `RelDataType` methods' javadoc), but just in case perhaps the most conservative approach would be removing the `isStruct()` check and just rely on the `AssertionError` thrown by `RelDataTypeImpl`.
Honestly, this is a minor change, I am open to any suggestion.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528642937
##########
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:
Committed new proposal taking into account @vlsi 's input
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2272: [CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2272:
URL: https://github.com/apache/calcite/pull/2272#discussion_r528616477
##########
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:
Just in case, there's `org.apache.calcite.piglet.DynamicTupleRecordType` which supports `getField(String)`, and it is not struct at the same time.
That is why I'm inlined `isStruct()` check must not be added.
----------------------------------------------------------------
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:
users@infra.apache.org