feat: update substrait-go to substrait v0.79.0#194
Conversation
Report explicit error if the types are not as expected
Removed nullability handling from ScalarType
| {"((20, 20), (-3, -3), (1, 1), (10,10), (5,5)) corr(my_col::fp32, col0::fp32) = 1::fp64", "mismatched input 'my_col'"}, | ||
| {"((20, 20), (-3, -3), (1, 1), (10,10), (5,5)) corr(col0::fp32, column1::fp32) = 1::fp64", "mismatched input 'column1'"}, | ||
| {"((20, 20), (-3, -3), (1, 1), (10,10), (5,5)) corr(my_col::fp32, col0::fp32) = 1::fp64", "mismatched input '::' expecting ')"}, | ||
| {"((20, 20), (-3, -3), (1, 1), (10,10), (5,5)) corr(col0::fp32, column1::fp32) = 1::fp64", "mismatched input '::' expecting ')"}, |
There was a problem hiding this comment.
Because of updates to the function test grammar, these now fail with a different error message.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #194 +/- ##
==========================================
- Coverage 68.79% 68.74% -0.06%
==========================================
Files 45 45
Lines 10367 10369 +2
==========================================
- Hits 7132 7128 -4
- Misses 2886 2892 +6
Partials 349 349 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| func (v *TestCaseVisitor) VisitBoolean(*baseparser.BooleanContext) interface{} { | ||
| return &types.BooleanType{Nullability: types.NullabilityRequired} |
There was a problem hiding this comment.
This used to work by setting the nullability to NullabilityRequired, and then the VisitDataType which invoked this would override the nullability. As types have the nullability attached directly to them, we can just set the nullability correctly here. Tests failed without these changes.
|
|
||
| // Package substraitgo contains the experimental go bindings for substrait | ||
| // (https://substrait.io). | ||
| // |
benbellick
left a comment
There was a problem hiding this comment.
LGTM! Only remaining comment is about whether we should actually panic instead of reporting the error. This is a non-blocking issue though so feel free to ignore.
| @@ -427,7 +427,7 @@ func (v *TestCaseVisitor) VisitNullArg(ctx *baseparser.NullArgContext) interface | |||
|
|
|||
| func (v *TestCaseVisitor) VisitBooleanArg(ctx *baseparser.BooleanArgContext) interface{} { | |||
| value := strings.ToLower(ctx.BooleanLiteral().GetText()) == "true" | |||
There was a problem hiding this comment.
This isn't this PR so no worries about fixing here, but seems a bit strange to treat everything that isn't "true" as false.
There was a problem hiding this comment.
I think this is okay because the parser already would have thrown an error if it was not "true" or "false". So you can ignore this comment!
| if ctx.I64() != nil { | ||
| return &types.Int64Type{Nullability: nullability} | ||
| } | ||
| v.ErrorListener.ReportVisitError(ctx, fmt.Errorf("invalid integer type %v", ctx.GetText())) |
There was a problem hiding this comment.
The codecove report is complaining about all of these lines never being hit by the test cases. If I understand the grammar correctly, I think that this is truly an impossible state, because to reach here, we had to have already parsed one of the above 4 int types. Thus, a panic may be a more informative move here because we can use it to communicate an impossible state.
That being said, I'm not so particular on this issue and it isn't blocking for me. (Same comment applies everywhere else we have v.ErrorListener added.)
There was a problem hiding this comment.
As discussed offline, it may actually make sense to leave v.ErrorListener here instead of panicking. This makes the code forwards compatible. If someone updates the grammar, but forgets to update the visitor logic, we won't panic.
substrait-go does not currently support lambdas, which are needed to handle the filter and transform function in this extension
| unsupportedExtensions = map[string]bool{ | ||
| "unknown.yaml": true, | ||
| // Missing support for func/lambda types | ||
| "functions_list.yaml": true, |
There was a problem hiding this comment.
@Slimsammylim we should be able to remove this after you finish your support for lambdas
No description provided.