Skip to content

Address review follow-ups on foreign join pushdown (#996) - #1001

Merged
adsharma merged 1 commit into
LadybugDB:mainfrom
adsharma:fix/foreign-pushdown-review-followup
Sep 20, 2026
Merged

adsharma merged 1 commit into
LadybugDB:mainfrom
adsharma:fix/foreign-pushdown-review-followup

Conversation

@adsharma

Copy link
Copy Markdown
Contributor

Follow-up to #996, which merged without these review changes.

  • Resolve the node-table ID column from the bound catalog entry's primary key instead of assuming it is the first foreign column; fall back to an id-named column before ordinal position. Without this, Iceberg/Unity Catalog node tables whose PK is not first join on the wrong column.
  • Simplify the src/dst endpoint predicate into an isEndpointColumn helper shared (by convention) with DuckDBCatalog::createForeignRelTable.
  • Document the all-or-nothing endpoint fallback and the unqualified table-name fallback trade-off; extract quote-stripping helpers.

No build/test run; clang-format-18 clean.

- Resolve the node-table ID column from the bound catalog entry's
  primary key instead of assuming it is the first foreign column;
  fall back to an `id`-named column before ordinal position.
- Simplify the src/dst endpoint predicate into an isEndpointColumn
  helper shared (by convention) with DuckDBCatalog.
- Document the all-or-nothing endpoint fallback and the unqualified
  table-name fallback trade-off; extract quote-stripping helpers.
@adsharma
adsharma merged commit 33567d5 into LadybugDB:main Sep 20, 2026
4 checks passed
@adsharma
adsharma deleted the fix/foreign-pushdown-review-followup branch September 20, 2026 04:33
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.

1 participant