- 
                Notifications
    
You must be signed in to change notification settings  - Fork 92
 
Gordon.hamilton/nested struct support #579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Gordon.hamilton/nested struct support #579
Conversation
fac05f2    to
    0957216      
    Compare
  
    690c298    to
    2f14645      
    Compare
  
    | 
           @gord02 you will need to make sure that the subject line and text of all commits in your pull request conform to the structure dictated by conventional commits. You can look at the output of the Lint commits for semantic-release build check to see problems flagged, and also the CONTRIBUTING guide for tips. When you have lots of commits that need fixing, it can sometimes be easier to squash them into fewer commits to reduce the number of changes you need to make.  | 
    
ef39861    to
    17119c8      
    Compare
  
    | 
           Does this PR replace #417 which seems to have tried to do a similar change?  | 
    
| 
           Hi @nielspardon! This PR completes and builds off of that PR yes. So it can be deleted.  | 
    
…ut literals for substrait proto
17119c8    to
    d799bdf      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a few comments on here so far. I think some of them might cause some reshuffling around so I'll save some of the more in-depth suggestions for the future. One other thing, it would be great if you could introduce some test which specifically demonstrates the logic you introduced where deserialized protos can handle either the presence of fields or values, but not both. I can imagine a test in which you constuct two protos, one via values and one via fields, and just demonstrate that once you render them both as POJOs, they are equal. That's just one idea though.
Anyways, thanks for all of your work! Let me know if anything I wrote isn't clear :)
| .object(ExtensionDdl.DdlObject.VIEW) | ||
| .build(); | ||
| 
               | 
          ||
| io.substrait.proto.Rel protoRel = relProtoConverter.toProto(command); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test has become a test for virtual tables instead of the intended Ddl test. You can see that the virtual table was just fed in as an input the ExtensionDdl builder as a viewDefinition. Were these changes actually necessary?
| ProtoExpressionConverter converter = | ||
| new ProtoExpressionConverter(lookup, extensions, virtualTableSchema.struct(), this); | ||
| List<Expression.StructLiteral> structLiterals = new ArrayList<>(virtualTable.getValuesCount()); | ||
| 
               | 
          
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of handling below the converion of proto -> expression for StructLiterals and StrutNested's inline, could we introduce method to handle conversions of Nested in ProtoExpressionConverter.java (once you do the above of having a Nested interface)? Similar to how it is handled for Literals. 
This PR is to change the substrait-java virtualTableScan to use nested structs in place of the now deprecated struct literals.
The issue
Key changes:
Testing:
Testing:
./gradlew test --tests io.substrait.isthmus.LogicalValuesTest --debug-jvm