Doris-Breakwater commented on issue #65988:
URL: https://github.com/apache/doris/issues/65988#issuecomment-5066021795
## Preliminary maintainer assessment
**Verdict: confirmed code-level performance bug on master, with a
high-confidence root cause.** The report contains enough evidence to triage
without additional logs. Current issue metadata has no labels, assignee, or
milestone. A fix is already proposed in #65989, but the current patch has a
Thrift contract problem that should be corrected before merge.
### Code-path confirmation
- BE derives `required_columns` from the materialized schema-scan slots and
sends it in `TGetTablesParams` (`SchemaScanOperatorX` ->
`SchemaTablesScanner::_get_new_table()`).
- FE normalizes that set in `FrontendServiceImpl.listTableStatus()` and
conditionally computes the optional status fields, but currently calls
`table.getComment()` unconditionally.
- Since #64263, `IcebergExternalTable.getComment()` calls `properties()`,
which calls `getIcebergTable()`. For the reported REST/session-catalog path
this reaches Iceberg `loadTable`, matching the supplied jstack.
- Therefore, a projection such as `TABLE_SCHEMA, TABLE_NAME` still performs
one metadata load per Iceberg table. The measured latency and request
amplification are consistent with this exact call chain.
- Backward compatibility is already handled by `needTableStatusColumn()`: if
an older BE omits `required_columns`, its value is `null` and FE continues to
fill the full status.
### Important correction for #65989
`TTableStatus.comment` is declared as `3: required string comment` in
`gensrc/thrift/FrontendService.thrift`. The current PR simply skips
`status.setComment(...)` when the column is not requested. That leaves the Java
field `null`; generated Thrift Java validation rejects a missing required
reference field before writing the response. The C++ scanner also accesses
`.comment` unconditionally because the wire contract says it is required.
Please keep the required field populated while pruning only the expensive
lookup, for example:
```java
status.setComment(needTableStatusColumn(requiredColumns, "TABLE_COMMENT")
? table.getComment()
: "");
```
Initializing the comment to `""` and overriding it only when requested is
equivalent. Changing the Thrift field to optional is unnecessary and would
broaden the compatibility surface.
### Recommended validation
1. Extend `FrontendServiceImplTest` so a request without `TABLE_COMMENT`
verifies `getComment()` is never called, the returned comment is the required
empty placeholder, and the result passes Thrift validation/serialization.
2. Add the positive case: requesting `TABLE_COMMENT` calls `getComment()`
and returns the real value. Also retain a case with `required_columns` unset to
cover older-BE behavior.
3. Extend `test_tables_status_column_pruning.groovy` with a lightweight
projection that excludes `col=TABLE_COMMENT` and a positive projection that
includes it. This validates propagation of the required-column set, although
the FE mock/serialization test is still needed to catch the current PR defect.
4. On the reporter's REST catalog, compare the corrected build against the
same query and record wall time plus `loadTable`/table-GET count. The expected
result is zero per-table `loadTable` calls when `TABLE_COMMENT` is not
projected; listing namespaces/tables may still require catalog requests.
### Missing information
Non-blocking for diagnosis: the exact master commit SHA used for
reproduction and the promised before/after timing/request-count result from the
patched build. No Doris profile is needed for this FE metadata-RPC issue.
Suggested next step: label this as a bug affecting Iceberg/external-catalog
metadata performance, request the Thrift-safe adjustment and tests on #65989,
then merge after the focused FE test and REST-catalog verification pass.
Breakwater-GitHub-Analysis-Slot: slot_f27888c8b907
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
