Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the authoritative 24-file patch on exact head 7709352fe84ee4ed04929b8ae0b2d60cc9fb6132. I am requesting changes for six distinct issues: ORC-default tables cannot produce fresh partition-statistics files; the authenticated table view performs a synchronized authentication transition per manifest entry; successful MySQL follower result sets are reported as errors and lose row accounting; an unknown commit outcome can leave every FE cache stale after a durable commit; Arrow Flight action results cannot cross a follower boundary; and zero-placeholder prepared execution loses binary row encoding when forwarded.
Critical checkpoint conclusions:
- Scope and existing context: the authoritative diff paths exactly match
pr_changed_files.txt; the live base/head still match the bundle; no existing review or inline thread duplicates any accepted issue. - Iceberg correctness and lifecycle: snapshot ancestry, catalog-generation retry fencing, single-shot commit behavior, worker I/O authentication, stream cleanup, and borrowed
FileIOownership were traced. Definite-failure orphan handling and same-snapshot descriptor reuse match Iceberg's upstream action contract; the durable-but-unknown refresh gap remains blocking. - Authorization and result compatibility: ALTER authorization precedes external loading/mutation. Direct MySQL and direct Arrow result delivery are sound, but the three independent follower/protocol failures below remain. All nine migrated action adapters preserve their prior one-row result shape.
- Dependency and generated artifacts: the Parquet runtime dependency, shaded Thrift ABI, Java/C++ namespace split, stale generated-class cleanup, and clean/incremental packaging paths are consistent; no additional artifact defect survived review.
- Validation: all final-round normal and risk-focused reviewers returned
NO_NEW_VALUABLE_FINDINGSon this exact six-comment set. Per the review-task constraint, no local build or tests were run.git diff --checkonly reports the generated fixture's conventional extra trailing newline. Live COMPILE, Cloud UT, and completed static checks pass; the current FE UT failures are unrelated Lance class-redefinition/JNI glibc environment failures with no changed-path overlap, while several jobs remain pending. - User focus:
review_focus.txtstates that there is no additional user-provided focus, so the complete PR was reviewed without a narrower focus override.
| // Keep the selected snapshot even if the table head changes during computation. | ||
| long snapshotId = snapshot.snapshotId(); | ||
| Table statisticsTable = new IcebergPartitionStatsTable(icebergTable, authenticator); | ||
| PartitionStatisticsFile file = PartitionStatsHandler.computeAndWriteStatsFile(statisticsTable, snapshotId); |
There was a problem hiding this comment.
[P1] Support ORC tables before exposing this action
Iceberg 1.11 chooses the statistics-file format from the supplied table's write.format.default, but its internal writer has providers only for Avro and Parquet. The new integration test demonstrates the consequence: reuse of an older file works, then the first new snapshot on an ORC-default table fails with unregistered internal data format: ORC. Doris already supports ORC Iceberg tables and the factory exposes this procedure without a format restriction, so please decouple the statistics-file format (for example, present Parquet through the scoped table view) or gate the action consistently, and replace the expected failure with successful ORC compute/readback coverage.
| } | ||
|
|
||
| @Override | ||
| public Snapshot snapshot(long snapshotId) { |
There was a problem hiding this comment.
[P1] Avoid one authentication transition per manifest entry
Iceberg 1.11 calls table.snapshot(entry.snapshotId()) inside its loop over every manifest entry. This override turns that in-memory metadata lookup into authenticator.execute -> Hadoop doAs/getUGI; the Kerberos UGI lookup is synchronized. Large tables can therefore perform millions of needless authentication transitions and serialize worker progress even though BaseTable.snapshot only reads captured TableMetadata. Keep authentication around the actual FileIO and lazy stream operations, but serve snapshot metadata without re-entering the authenticator, and add an invocation-count regression test.
| byte[] bytes = new byte[definition.remaining()]; | ||
| definition.get(bytes); | ||
| Assertions.assertTrue(new String(bytes, StandardCharsets.UTF_8).contains("partition_statistics_file")); | ||
| Assertions.assertEquals(QueryState.MysqlStateType.EOF, state.getStateType()); |
There was a problem hiding this comment.
[P1] Exercise and fix the complete follower path
This assertion captures the state that breaks forwarding: sendResultSet leaves a successful result at EOF, while ConnectProcessor.proxyExecute assigns status 0 and affectedRows only for OK. A follower still replays these packet buffers, so this test passes, but it receives ERR_UNKNOWN_ERROR, loses returned-row accounting, records an audit error, and stops any remaining multi-statements. Please cover the complete master-to-follower path for 0/1/multiple rows, treat a successful result-set EOF as proxy success, and carry the real returned-row count.
| icebergTable.updatePartitionStatistics().setPartitionStatistics(file).commit(); | ||
| Env.getCurrentEnv().getExtMetaCacheMgr().invalidateTableCache(dorisTable); | ||
| return Collections.singletonList(Collections.singletonList(file.path())); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
[P1] Refresh metadata after an unknown commit outcome
A CommitStateUnknownException can be raised after the metadata update is durable; the new integration test simulates exactly that and observes the registered statistics entry. This catch path leaves the local invalidation unexecuted and also prevents ExecuteActionCommand from publishing its follower refresh log, so every FE can keep serving pre-commit metadata after a real commit. Do not retry the mutation or delete the file, but conservatively invalidate and propagate the refresh before rethrowing, and make the command-level test require those cache/edit-log interactions.
| for (int rowCount : new int[] {0, 1, 2}) { | ||
| ConnectContext ctx = Mockito.mock(ConnectContext.class); | ||
| ProxyMysqlChannel channel = new ProxyMysqlChannel(); | ||
| Mockito.when(ctx.getConnectType()).thenReturn(ConnectType.MYSQL); |
There was a problem hiding this comment.
[P1] Support Arrow Flight clients on follower FEs
ExecuteActionCommand is ForwardWithSync, but this new result path is tested only with a MySQL proxy channel. On a non-master FE, an Arrow Flight connection fails while building the forward request because buildStmtForwardParams unconditionally calls ctx.getMysqlChannel().clientDeprecatedEOF(), and FlightSqlConnectContext.getMysqlChannel() throws. Merely guarding that read is not enough: the request carries no result protocol and the master returns MySQL packet buffers rather than a Flight ResultSet. The action therefore works or fails according to which FE receives the same Flight request. Please make forwarding/result transport protocol-aware (or explicitly route/reject this command) and cover empty, one-row, and multi-row results through a follower Flight connection.
| } | ||
| ResultSet result = new CommonResultSet(new CommonResultSetMetaData(Collections.singletonList( | ||
| new Column("partition_statistics_file", PrimitiveType.STRING, true))), rows); | ||
| new StmtExecutor(ctx, new OriginStatement("", 0), true).sendResultSet(result); |
There was a problem hiding this comment.
[P1] Preserve binary mode for zero-parameter prepared executes
The common compute_partition_stats() call has no placeholders. For a server-prepared execution, handleExecute saves prepareExecuteBuffer only when paramCount > 0, so the follower omits it from TMasterOpRequest; the master then takes queryRetry() through the text-protocol proxy constructor and serializes result rows with sendTextResultRow(). The client is waiting for binary rows, whose first byte/null bitmap differ. This test uses that same text-only constructor and checks only packet count/metadata, so it cannot catch the corruption. Forward an explicit COM_STMT_EXECUTE/binary-result flag independently of parameter bytes, and assert binary row framing for a zero-argument action on a follower.
FE UT Coverage ReportIncrement line coverage |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
hubgeter
changed the title
Allow BaseExecuteAction to return a list of rows, including zero rows with column metadata. Adapt all existing Iceberg actions to keep their original single-row results. Cover empty and multi-row results, row validation, and forwarding through ProxyMysqlChannel.
Add ALTER TABLE ... EXECUTE compute_partition_stats with optional snapshot_id. Delegate statistics computation and registration to the Iceberg SDK, return the statistics file path or an empty result set, and invalidate the local table cache after commit. Provide the Parquet runtime module and isolate generated Java Thrift classes from official Parquet classes. Add action, SDK, command, and SQL regression coverage.
bb17822 to
7027db0
Compare
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Doris does not expose a maintenance command to compute and register Iceberg
partition statistics. Add
ALTER TABLE ... EXECUTE compute_partition_stats,with an optional
snapshot_idthat defaults to the current snapshot. Use theIceberg 1.11.0 SDK to compute or reuse a partition statistics file, register it
in table metadata, invalidate the local cache after commit, and return its path.
Extend
BaseExecuteActionto support zero or multiple result rows so thisprocedure can return zero rows with its column metadata intact when no statistics
file is produced. Adapt the nine existing Iceberg actions to the new internal
return type while preserving their existing single-row schemas and values.
Include the Parquet runtime reader/writer required by the SDK. Move Doris-generated
Parquet Thrift Java classes into a separate namespace and remove legacy generated
sources/classes during source generation to prevent conflicts with the official
Parquet classes. The Thrift wire definitions and generated C++ content are unchanged.
Release note
Add an Iceberg
compute_partition_statsprocedure to compute and registerpartition statistics for the current or a specified snapshot.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)