Conversation
8485c22 to
f613e7a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Legacy CSS metadata remains incompatible, and the query-analysis behavior lacks coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds cross-database director resolution for ref-match tables.
Changes:
- Extends match metadata with director database names.
- Preserves this metadata through ingest and CSS.
- Resolves directors using their specified databases.
File summaries
| File | Description |
|---|---|
src/replica/contr/HttpIngestModule.cc |
Publishes director database metadata. |
src/qana/TableInfoPool.cc |
Resolves cross-database directors. |
src/css/testCssAccess.cc |
Tests CSS metadata persistence. |
src/css/MatchTableParams.h |
Adds director database fields. |
src/css/CssAccess.cc |
Reads and writes the new fields. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| infoPtr->director.first = | ||
| dynamic_cast<DirTableInfo const*>(get(m.dirDb1.empty() ? db_ : m.dirDb1, m.dirTable1)); | ||
| infoPtr->director.second = | ||
| dynamic_cast<DirTableInfo const*>(get(m.dirDb2.empty() ? db_ : m.dirDb2, m.dirTable2)); |
iagaponenko
left a comment
iagaponenko
left a comment
There was a problem hiding this comment.
This looks good to me. However, we should add a test case (a dataset) to test this feature in the integration test.
There was a problem hiding this comment.
🟡 Changes recommended
Table-configuration export still drops director database qualifiers, breaking export and re-ingest of the newly supported tables.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 31/34 changed files
- Comments generated: 1
- Review effort level: Balanced
`test_cases` entries can now include an optional `databases` key that allows several database `root`, `data`, `schema`, etc to be listed. If not present, the previous behavior is used (one database per integration test).
caa7221 to
d0969db
Compare
Previously, ref-match tables were assumed to always reference tables within their own database. If a table was ingested that referenced a director in a different database than its own, it would fail query analysis. This adds support for ref-match tables that reference directors outside its own database.
Summary of changes
MatchTableParamshas two new members,dirDb1anddirDb2to track director database names (if they exist)TableInfoPoolnow checks for the director database name when populatingMatchTableParamsdata/case03