substrait-io / substrait-java

Apache License 2.0
72 stars 70 forks source link

feat: convert sql expression into proto extended expressions #191

Closed davisusanibar closed 5 months ago

davisusanibar commented 8 months ago

The proposal involves receiving a SQL expression (e.g. column_a > 18), validating the SqlNode expression, and then converting it into RexNode. It then traverses RexNode to validate the type of expression, and finally creates a proto Extended Expression message.

These are the activities involved on this PR:

These are the activities pending to solve on this PR:

Please let me know if you have any ideas, improvements, or concerns so that I can evaluate them appropriately. I appreciate your assistance in advance.

To closes: https://github.com/substrait-io/substrait-java/issues/128

davisusanibar commented 7 months ago

The PR is ready for review. It therefore depends on https://github.com/substrait-io/substrait-java/pull/206

davisusanibar commented 6 months ago

Hi @vbarua thank you for the code review, the comments were addressed.

Additionally please if you could give some reason of the current error on the CI build (for some reason I am not able to replicate that locally):

> Task :core:generateTestProto NO-SOURCE

/home/runner/work/substrait-java/substrait-java/core/src/test/java/io/substrait/type/proto/TestTypeRoundtrip.java:54: warning: as of release 10, 'var' is a restricted type name and cannot be used for type declarations or as the element type of an array
    var converted = type.accept(typeProtoConverter);
        ^
> Task :core:compileTestJava FAILED
/home/runner/work/substrait-java/substrait-java/core/src/test/java/io/substrait/type/proto/LocalFilesRoundtripTest.java:24: warning: as of release 10, 'var' is a restricted type name and cannot be used for type declarations or as the element type of an array
:core:compileTestJava failure marker
vbarua commented 6 months ago

Additionally please if you could give some reason of the current error on the CI build

I'm not entirely sure either unfortunately. In some case Jabel doesn't seem to fire correctly and it doesn't transform code that it should be. It happens more than I would like, see https://github.com/substrait-io/substrait-java/issues/195

I tried making a small change in https://github.com/substrait-io/substrait-java/pull/191/commits/3835a4f35d41ac31dc9dfcd9d0fb4876602a002e to retry the build, which sometimes works, but apparently not in this case. If the CI check is failing after we've approved the PR we can figure it out then.

vibhatha commented 5 months ago

Additionally please if you could give some reason of the current error on the CI build

I'm not entirely sure either unfortunately. In some case Jabel doesn't seem to fire correctly and it doesn't transform code that it should be. It happens more than I would like, see #195

I tried making a small change in 3835a4f to retry the build, which sometimes works, but apparently not in this case. If the CI check is failing after we've approved the PR we can figure it out then.

@vbarua would be a best practice to keep the usage of higher SDK features minimal and stick to basics at most cases?

davisusanibar commented 5 months ago

Thank you @vbarua / @vibhatha / @danepitkin for your comments/suggestion.

Please if you could help me with a new code review.

PD: PR related to support qualified path on Calcite SQL Expression was created.

vibhatha commented 5 months ago

PD: PR related to support qualified path on Calcite SQL Expression was created. :fireworks: :+1:

vibhatha commented 5 months ago

@vbarua should we cover more ground on tests? Not necessarily for this PR itself but for the sustainability in the longer term?