Skip to content

Fixes 33353: Batch table custom-metric and column-extension reads - #33355

Draft
harshach wants to merge 1 commit into
mainfrom
harshach/table-metadata-batch-reads
Draft

harshach wants to merge 1 commit into
mainfrom
harshach/table-metadata-batch-reads

Conversation

@harshach

Copy link
Copy Markdown
Collaborator

Describe your changes:

Fixes #33353

Part of #32946, shipped on its own as the reviewer suggested: this is the "505 → 5 statements on table metrics" win from #33248, ported onto the unchanged EntityRepository so it can merge and cherry-pick today.

Table detail reads fetched custom metrics once per column and column extensions once per column, so GET /v1/tables/{id}?fields=columns,customMetrics on a 100-column table ran 101 entity_extension queries. TableMetadataLoader now loads table and column metrics for a whole batch of tables with one prefix query, and column extensions with one keyed query (getExtensionsByKeys, chunked like the other IN lists), so the per-request count no longer depends on column width. The paginated column endpoints and the single-column read share the same loader, and TableRepository loses its four per-column readers.

Type of change:

  • Improvement

High-level design:

  • TableMetadataLoader (new, jdbi3): three read operations, one query each: loadMetrics(tables, includeColumns), loadColumnMetrics(tableId, columns), loadColumnExtensions(tableId, columns). It takes a Supplier<EntityExtensionDAO> so the reads join whatever transaction the caller already holds.
  • EntityExtensionDAO.getExtensionsByKeys: WHERE id = :id AND extension IN (<keys>), chunked through EntityDAO.queryInChunks. Column extensions are looked up by their exact persisted key, so legacy rows stored under another jsonSchema are still returned (the paginated path already read by key).
  • TableRepository: setFields, fetchAndSetCustomMetrics, the two paginated column reads and the single-column read call the loader; getCustomMetrics, batchFetchCustomMetrics, batchFetchCustomMetricsByColumn and getColumnExtension are deleted. The public constants keep their values (they now alias the loader's).
  • No public/protected repository API changes; Collate compiles unchanged. Response JSON is unchanged: metric order per column is still the extension-name order, and absent metrics still serialize as [].
  • SqlQueryCounter (test util) is byte-identical to the one in Fixes 32946: Pin entity repository lifecycle and cache contracts (1/7) #33344 so the two PRs merge in either order.

Tests:

Use cases covered

  • GET /v1/tables/{id}?fields=columns,customMetrics at 3, 100 and 1,000 columns: one extension query, table metric and column metric both present, unrelated customMetrics.table.tableish.* rows ignored.
  • fields=columns,extension: one table-extension query plus one keyed query; a legacy column-extension row stored under another jsonSchema is returned.
  • fields=columns alone runs zero extension queries and leaves customMetrics null.
  • Bulk setFieldsInBulk across two tables with the same column names: one query, metrics not mixed across tables.
  • GET /v1/tables/{id}/columns?fields=customMetrics&limit=3: one extension query, counted only inside the HTTP request.
  • A metric inserted and read inside a rolled-back transaction is visible inside it and gone after it.

Unit tests

  • TableMetadataLoaderTest (8 tests, no database): absent inputs need no DAO, selected-column filtering, table-only reads do not decode column metrics, overlapping column names across tables stay distinct, malformed or unresolvable keys do not stop later columns, DAO failure clears stale values.

Backend integration tests

  • TableMetadataReadIT (8 tests, real SQL statement counter, @Isolated).
  • Failing-before evidence, same test class against main's TableRepository (PostgreSQL): 5 of 8 fail on the query budget alone: 3 columns → 4 queries, 100 → 101, 1,000 → 1,001; fields=columns,extension → 101; two-table bulk read → 202. The three that pass before (fields=columns runs zero queries, the paginated page already ran one, the transaction check) are regression guards.
  • After: 8 passed on PostgreSQL/OpenSearch and 8 passed on MySQL/Elasticsearch (Java 21, Testcontainers).
  • Existing coverage of the touched paths, PostgreSQL/OpenSearch: TableResourceIT (294 cases, including createUpdateDelete_tableCustomMetrics_200 and test_getTableColumnsWithCustomMetrics_200) and ColumnCustomPropertiesIT (65 cases): 364 run, 0 failures, 0 errors, 9 pre-existing skips.

Ingestion integration tests

Not applicable (no ingestion changes).

Playwright (UI) tests

Not applicable (no UI changes).

Manual testing performed

None beyond the automated runs above; all evidence is from the integration tests listed.

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: not applicable, no schema changes.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added tests around the new logic.

Table detail reads fetched custom metrics once per column and column
extensions once per column, so a 100-column table with customMetrics
cost 101 entity_extension queries per request. TableMetadataLoader now
loads table and column metrics for a whole batch of tables with one
prefix query, and column extensions with one keyed query, keeping the
per-request count independent of column width. The paginated column
endpoints share the same loader.

Part of #32946

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Sep 15, 2026
@@ -2936,11 +2877,7 @@ private ResultList<Column> getTableColumnsInternal(
}

if (fieldsParam != null && fieldsParam.contains("customMetrics")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Bug: Paginated /columns extension read not routed through loader

In getTableColumns, the customMetrics branch was converted to metadataLoader.loadColumnMetrics, but the adjacent extension branch (lines 2883-2906) still reads via getExtensionsByJsonSchema(id, COLUMN_EXTENSION_JSON_SCHEMA), filtering on jsonschema = 'columnExtension'. The new loadColumnExtensions used by setFields and the single-column read (enrichSingleColumnFields) resolves by the exact persisted key regardless of jsonSchema, so a legacy column-extension row stored under a different jsonSchema will be returned by GET /v1/tables/{id}?fields=columns,extension and the single-column read but NOT by the paginated GET /v1/tables/{id}/columns?fields=extension. This contradicts the PR's stated claim that "the paginated path already read by key." Consider routing this branch through metadataLoader.loadColumnExtensions(table.getId(), paginatedColumns) for consistency.

Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Batch table custom-metric and column-extension reads reduce per-request queries from 101 down to 1 across 100-column tables, with comprehensive test coverage. Consider routing the paginated /columns extension read through the new loader for consistency with single-column and bulk reads, which resolve by exact key rather than jsonSchema filter.

💡 Bug: Paginated /columns extension read not routed through loader

📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java:2879-2893

In getTableColumns, the customMetrics branch was converted to metadataLoader.loadColumnMetrics, but the adjacent extension branch (lines 2883-2906) still reads via getExtensionsByJsonSchema(id, COLUMN_EXTENSION_JSON_SCHEMA), filtering on jsonschema = 'columnExtension'. The new loadColumnExtensions used by setFields and the single-column read (enrichSingleColumnFields) resolves by the exact persisted key regardless of jsonSchema, so a legacy column-extension row stored under a different jsonSchema will be returned by GET /v1/tables/{id}?fields=columns,extension and the single-column read but NOT by the paginated GET /v1/tables/{id}/columns?fields=extension. This contradicts the PR's stated claim that "the paginated path already read by key." Consider routing this branch through metadataLoader.loadColumnExtensions(table.getId(), paginatedColumns) for consistency.

🤖 Prompt for agents
Code Review: Batch table custom-metric and column-extension reads reduce per-request queries from 101 down to 1 across 100-column tables, with comprehensive test coverage. Consider routing the paginated `/columns` extension read through the new loader for consistency with single-column and bulk reads, which resolve by exact key rather than `jsonSchema` filter.

1. 💡 Bug: Paginated /columns extension read not routed through loader
   Files: openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/TableRepository.java:2879-2893

   In `getTableColumns`, the `customMetrics` branch was converted to `metadataLoader.loadColumnMetrics`, but the adjacent `extension` branch (lines 2883-2906) still reads via `getExtensionsByJsonSchema(id, COLUMN_EXTENSION_JSON_SCHEMA)`, filtering on `jsonschema = 'columnExtension'`. The new `loadColumnExtensions` used by `setFields` and the single-column read (`enrichSingleColumnFields`) resolves by the exact persisted key regardless of `jsonSchema`, so a legacy column-extension row stored under a different `jsonSchema` will be returned by `GET /v1/tables/{id}?fields=columns,extension` and the single-column read but NOT by the paginated `GET /v1/tables/{id}/columns?fields=extension`. This contradicts the PR's stated claim that "the paginated path already read by key." Consider routing this branch through `metadataLoader.loadColumnExtensions(table.getId(), paginatedColumns)` for consistency.

Review coverage

Rules No rules evaluated

Functional validation Not enabled · Set up

Auto-approval Not enabled · Set up

Options

Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Table custom-metric and column-extension reads scale with column count

1 participant