Skip to content
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

TableName extraction fails for tables with name pattern as db.tablename in CalciteSqlParser #10701

Closed
abhioncbr opened this issue Apr 28, 2023 · 6 comments

Comments

@abhioncbr
Copy link
Contributor

extractTableNamesFromNode extracts incorrect name for the table with a pattern like db.tablename.

Instead of extracting the full name as one value, it's outputting two values. Example query

Select * from db.tbl1

It extracts a List with 2 elements (1. 'db', 2. 'tbl1')

SqlNodeAndOptions for the above query has two values in the FROM node.
Screen Shot 2023-04-28 at 7 25 09 PM

Just confirming, is it suppose to be like this?

cc @walterddr @ankitsultana @tibrewalpratik17

@ankitsultana
Copy link
Contributor

From Calcite's perspective this is correct and you can refer to RelOptTable.

I am not sure if we allow dot separated table names in Pinot.

@abhioncbr
Copy link
Contributor Author

From Calcite's perspective this is correct and you can refer to RelOptTable.

I am not sure if we allow dot separated table names in Pinot.

Thanks for the confirmation.
I came to know about this because one of the integration test case failed with the usage of the above method.

@walterddr
Copy link
Contributor

this is actually not suppose to work. to a complete SQL system db.table1 means {"catalog": "db", "table": "table1"}
however pinot doesn't support catalog thus any prepended catalog is resolved as "default" thus the integration test case is to verify that for the existing query engine.

However for the new engine, we actually implicitly added the default catalog name since Calcite's catalog reader requires a root schema (or root catalog if you will) thus it will not be able to find a db catalog internally.

We are yet to discuss the catalog support going forward but as far as for now this will not be supported as "ignoring the catalog and only parse table name, then move on with what we had" is not really something we should do

@navina
Copy link
Contributor

navina commented May 16, 2023

@walterddr : is there an action item on this issue?

follow-up question: Have we already documented the rules / requirements for a pinot table resource somewhere?

@walterddr
Copy link
Contributor

walterddr commented May 16, 2023

  • I don't think so at this moment. we can continue discussion this thread on how to support catalog going forward.
  • I couldn't find a single reference to db.tbl or catalog.tbl in pinot official doc. so the usage pattern is probably just "worked unintentionally" and ppl start using it. and I am not sure this is the pattern we want to do going forward
  • we can check how other ANSI SQL system works with catalog but AFAIK I don't think any will 'fallback to default catalog if tbl name is missing in that explicitly specified catalog'; i do know some will use default catalog if catalog was unspecified though

@shounakmk219
Copy link
Collaborator

@abhioncbr The mentioned issue should be addressed by #12591
Tagging @gortiz as well.

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

No branches or pull requests

5 participants