Namanmahor/plat 427 unify the informationschema and olapinformationschema#9237
Namanmahor/plat 427 unify the informationschema and olapinformationschema#9237NamanMahor wants to merge 26 commits into
Conversation
…nify-the-informationschema-and-olapinformationschema
…ema-and-olapinformationschema
…ema-and-olapinformationschema
…ltDatabaseSchema for TestListTable and TestLookUp
|
Thanks for the changes @NamanMahor , there wont be confusion on what to use now. Do we also plan to also unify |
I will merge this PR and then After application change will remove OLAPListTables. |
| func (c *connection) LoadPhysicalSize(ctx context.Context, tables []*drivers.OlapTable) error { | ||
| // already populated in All and Lookup calls | ||
| func (c *connection) LoadPhysicalSize(ctx context.Context, tables []*drivers.TableInfo) error { | ||
| // already populated in Lookup calls |
There was a problem hiding this comment.
This is not correct for ListTables. db.Schema seems to be populating size, ListTables is querying directly.
Found this while doing a sanity test on an existing duckdb project, old API returns size where new doesnt even with loadPhysicalSize: true
There was a problem hiding this comment.
sure will fix it.
| } | ||
|
|
||
| if like != "" { | ||
| filter += " AND (LOWER(T.name) LIKE LOWER(?) OR CONCAT(T.database, '.', T.name) LIKE LOWER(?))" |
There was a problem hiding this comment.
Why was this removed ? This seems like a breaking change ?
| `, prefix, prefix) | ||
|
|
||
| db, err := c.getDB(ctx) | ||
| conn, err := c.getDB(ctx) |
There was a problem hiding this comment.
Why rename from db to conn ?
conn gives an impression that it needs to be closed as well ?
|
|
||
| func (c *connection) ListTables(ctx context.Context, database, databaseSchema string, pageSize uint32, pageToken string) ([]*drivers.TableInfo, string, error) { | ||
| // ListTables implements drivers.InformationSchema. | ||
| // DuckDB connections are bound to a single active database and schema, so the database and databaseSchema |
| WHERE ` | ||
|
|
||
| var args []any | ||
| q += "table_schema = ?" |
There was a problem hiding this comment.
This is without any condition so why is this done separately(as compared to earlier change) ?
| } | ||
| q += " AND table_name > ?" | ||
| args = append(args, startAfter) | ||
| q += " AND (table_schema > ? OR (table_schema = ? AND table_name > ?))" |
There was a problem hiding this comment.
If the schema is always pinned to a single schema than why is this check required. Wouldn't the earlier table_schema be sufficient ?
| } | ||
|
|
||
| func (c *connection) ListTables(ctx context.Context, database, databaseSchema string, pageSize uint32, pageToken string) ([]*drivers.TableInfo, string, error) { | ||
| func (c *connection) ListTables(ctx context.Context, database, databaseSchema, like string, pageSize uint32, pageToken string) ([]*drivers.TableInfo, string, error) { |
There was a problem hiding this comment.
Same comment as made in other place as well. Schema is already pinned.
| return nil, "", fmt.Errorf("like filter not supported") | ||
| } | ||
| // AllFromInformationSchema is a helper that drivers can use to implement All() by iterating ListDatabaseSchemas + ListTables. | ||
| func AllFromInformationSchema(ctx context.Context, like string, pageSize uint32, pageToken string, i InformationSchema) ([]*TableInfo, string, error) { |
There was a problem hiding this comment.
This has a somewhat weird implementation right now.
It list the schemas limited by the page size parameter and then searches in individual schemas.
I would expect an All API to fetch me all tables that match a pattern across all schemas/databases in a paginated fashion like clickhouse does on main.
…ema-and-olapinformationschema
…ema-and-olapinformationschema
3 participants
PLAT-427: Unify the
InformationSchemaandOLAPInformationSchemainterfacesChecklist: