Skip to content

Conversation

@rymurr
Copy link
Contributor

@rymurr rymurr commented May 23, 2022

This uses a function to look up a table schema without a create table statement. I've intentionally kept the interface free from calcite objects so as to not export them as part of the isthmus public API.

I am not an expert at calcite so I am not sure if this is the best way to do it. Also, note, my fix adds state to the converter. Happy to adjust based on input.

@jacques-n
Copy link
Collaborator

Hey @rymurr, I don't think this is the right pattern. As part of validation, the validator needs to figure out resolution based on current schema versus root schema. I also think it may be the case that some of the identifiers you'd find in nested queries wouldn't be tables (not sure on that). I think you need to plugin closer to the schema apis. That being said, I'm not sure the best pattern to do so. I remember at Dremio we actually had a fork of Calcite for exactly this purpose. Maybe @laurentgo would have some advice on whether those changes have been pushed back into Calcite using some kind of pattern.

@jinfengni
Copy link
Contributor

Seems to me we need CalciteSchema that dynamically loads subschema/tables on demand when the validator requests to validate an identifier either for subschema/table. The tableLookUp function should be provided to such dynamic CalciteSchema.

This pr first finds all the sql identifier in the parse tree, and then try to match/find the identifier in tableLookUp. However, an identifier abc could be either a subschema name, or table name or column name. We would not know which kind it is, unless we have validator's context.

@rymurr
Copy link
Contributor Author

rymurr commented May 24, 2022

Thanks a lot for the feedback @jinfengni and @jacques-n

I have updated to use a catalog. Do you think this is a better approach?

@rymurr
Copy link
Contributor Author

rymurr commented May 24, 2022

Thanks for the review @jacques-n caught a few bugs and fixed them in the most recent iteration.

@jacques-n
Copy link
Collaborator

This looks reasonable to me. Merging.

@jacques-n jacques-n merged commit e886c6f into substrait-io:main May 26, 2022
@rymurr rymurr deleted the table-lookup branch May 26, 2022 09:25
ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024
* Add table lookup entry point to SqlToSubstrait

Co-authored-by: Jacques Nadeau <jacquesnadeau@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants