[CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression #2272

rubenada · 2020-11-19T16:05:47Z

zabetak · 2020-11-23T08:50:34Z

core/src/main/java/org/apache/calcite/tools/RelBuilder.java

+    if (!e.getType().isStruct()) {
+      throw new IllegalArgumentException("Requested field " + name + " in non-struct expression "
+          + e.toString() + " (type: " + e.getType() + ")");
+    }


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.

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.

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.

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.

@vlsi we had the same thought :)

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)?

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:

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.

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

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.

Ah, indeed, RelCrossType is a case when isStruct == false and getFieldList() returns non-null, so .isStruct() check must not be added to RexBuilder.

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

Committed new proposal taking into account @vlsi 's input

danny0405 · 2020-11-23T13:08:34Z

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);


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".

@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

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"

Exception message reviewed to make it more high level

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.

Exceptions should always be meaningful, even the low-level ones.

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.

Please suggest exact pattern to insert into new IllegalStateException(...)

rubenada · 2020-11-25T08:07:08Z

@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?

julianhyde · 2020-11-25T15:52:40Z

@rubenada I think the message should be simple and terse. Less is more.

julianhyde · 2020-11-25T16:00:38Z

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 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/src/main/java/org/apache/calcite/rel/type/RelDataTypeImpl.java <#2272 (comment)>: > @@ -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 <#2272 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIUAOIUJYNEZQ3R736LZC3SRUSRBANCNFSM4T3TA5EQ> .

rubenada · 2020-11-25T16:16:51Z

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.

danny0405 · 2020-11-26T02:06:44Z

core/src/test/java/org/apache/calcite/test/RelBuilderTest.java

+    }, "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")));


What is the purpose to split into two containsString here ?

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.

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

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?

Ok, changed.

…eld on a non-struct expression

danny0405

+1, looks fine from my side.

rubenada · 2020-11-26T11:25:46Z

Merged.
Thanks everyone for the feedback.

rubenada requested a review from zabetak November 20, 2020 10:42

zabetak reviewed Nov 23, 2020

View reviewed changes

danny0405 reviewed Nov 24, 2020

View reviewed changes

rubenada force-pushed the CALCITE-4409 branch from 3ead64c to 367181d Compare November 25, 2020 10:46

rubenada force-pushed the CALCITE-4409 branch from 0ad1bc4 to 7fc50b0 Compare November 25, 2020 16:14

rubenada added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Nov 25, 2020

danny0405 reviewed Nov 26, 2020

View reviewed changes

[CALCITE-4409] Improve exception when RelBuilder tries to create a fi…

2264654

…eld on a non-struct expression

rubenada force-pushed the CALCITE-4409 branch from bcbf748 to 2264654 Compare November 26, 2020 08:36

danny0405 approved these changes Nov 26, 2020

View reviewed changes

vlsi approved these changes Nov 26, 2020

View reviewed changes

rubenada merged commit fc912eb into apache:master Nov 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression #2272

[CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression #2272

rubenada commented Nov 19, 2020

zabetak Nov 23, 2020

danny0405 Nov 23, 2020

rubenada Nov 23, 2020 •

edited

vlsi Nov 23, 2020

rubenada Nov 23, 2020

rubenada Nov 23, 2020

vlsi Nov 23, 2020 •

edited

vlsi Nov 23, 2020 •

edited

rubenada Nov 23, 2020 •

edited

rubenada Nov 23, 2020

danny0405 Nov 23, 2020 •

edited

rubenada Nov 24, 2020

vlsi Nov 25, 2020

rubenada Nov 25, 2020

julianhyde Nov 25, 2020

vlsi Nov 25, 2020

julianhyde Nov 25, 2020

vlsi Nov 25, 2020

rubenada commented Nov 25, 2020

julianhyde commented Nov 25, 2020

julianhyde commented Nov 25, 2020 via email

rubenada commented Nov 25, 2020

danny0405 Nov 26, 2020

rubenada Nov 26, 2020

vlsi Nov 26, 2020

rubenada Nov 26, 2020

vlsi Nov 26, 2020

rubenada Nov 26, 2020

danny0405 left a comment

rubenada commented Nov 26, 2020

[CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression #2272

[CALCITE-4409] Improve exception when RelBuilder tries to create a field on a non-struct expression #2272

Conversation

rubenada commented Nov 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenada Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlsi Nov 23, 2020 • edited

Choose a reason for hiding this comment

vlsi Nov 23, 2020 • edited

Choose a reason for hiding this comment

rubenada Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenada commented Nov 25, 2020

julianhyde commented Nov 25, 2020

julianhyde commented Nov 25, 2020 via email

rubenada commented Nov 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danny0405 left a comment

Choose a reason for hiding this comment

rubenada commented Nov 26, 2020

rubenada Nov 23, 2020 •

edited

vlsi Nov 23, 2020 •

edited

vlsi Nov 23, 2020 •

edited

rubenada Nov 23, 2020 •

edited

danny0405 Nov 23, 2020 •

edited