refactor: extract getUsedMembers to mirror getUsedTableSchema#254
Merged
Conversation
Addresses review feedback on #252: - Move the filter/order/measures/dimensions walker out of ensure-table-schema-alias-sql.ts into src/get-used-members/, mirroring the structure of src/get-used-table-schema/. - Reuse traverseMeerkatQueryFilter for the filter traversal so we share a single walker with getUsedTableSchema and get a properly typed path through MeerkatQueryFilter (drops the prior `unknown`-cast workaround). - Drop duplicate tests from ensure-table-schema-alias-sql.spec.ts that are now owned by get-used-members.spec.ts; keep the one integration test that asserts the aliaser filters descriptors end-to-end. - Add brief doc comments to ensureTableSchemaAliasSql explaining why TableSchema.joins[].sql is preserved as-is, and to the struct-field helpers clarifying what `knownTableNames` does (optional for the legacy fallback path). Issue: ISS-299745
shriram-devrev
requested review from
itsTalwar,
ujaval403,
vpbs2 and
zaidjan-devrev
as code owners
zaidjan-devrev
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #252 — addresses review feedback from @zaidjan that didn't make it into the squash merge.
Two valid cleanups:
unknown-cast filter walker. The inlinecollectFilterMembershelper inensureTableSchemaAliasSqlused a structural cast to sidestep narrowing issues withMeerkatQueryFilter.getUsedTableSchema.getUsedTableSchemaalready walks the same fields (measures/dimensions/order/filters) to collect table names. This adds the member-level parallel.What changed
New file:
meerkat-core/src/get-used-members/get-used-members.ts—getUsedMembers(query: Query): Set<Member>that reuses the existingtraverseMeerkatQueryFilterhelper (same onegetUsedTableSchemauses).get-used-members.spec.ts— 7 unit tests (measures/dims, order, flat + nested filters, joinPaths-ignored, union, empty).Refactored:
meerkat-core/src/utils/ensure-table-schema-alias-sql.tscollectReferencedMembers+collectFilterMembers(theunknown-cast walker).getUsedMembers(query)— proper typing, shared traversal withgetUsedTableSchema.ensureTableSchemaAliasSqlnotingTableSchema.joins[].sqlis preserved as-is.Pruned:
ensure-table-schema-alias-sql.spec.tsget-used-members.spec.ts(filter-only, order-only, joinPaths-ignored).Doc comments:
ensure-sql-expression-column-alias.tsisAliasableIdentifier(what it accepts).shouldEnsureColumnRefAliasexplaining whyknownTableNamesis optional (fallback when schema batch is absent).ensureOrderByNodesAliasexplaining why the param is threaded through.Behavior change
None. Pure refactor — same inputs produce identical outputs.
Test plan
nx test meerkat-core— 56 suites / 538 tests (+1 new suite, +7 new tests, -3 pruned duplicates)nx test meerkat-browser— unchanged, 2/2 passnx test meerkat-node— unchanged, 887/887 passnx run-many -t build -p meerkat-core,meerkat-browser,meerkat-node— cleanRelated