github-actions[bot] commented on code in PR #66345:
URL: https://github.com/apache/doris/pull/66345#discussion_r3701224039
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java:
##########
@@ -257,6 +346,12 @@ public void appendExplainInfo(StringBuilder output, String
prefix,
@Override
public List<ConnectorWriteSortColumn> getWriteSortColumns(ConnectorSession
session,
ConnectorTableHandle tableHandle) {
+ return getWriteSortColumns(session, tableHandle,
Collections.emptyList());
+ }
+
+ @Override
+ public List<ConnectorWriteSortColumn> getWriteSortColumns(ConnectorSession
session,
+ ConnectorTableHandle tableHandle, List<ConnectorColumn>
boundTargetColumns) {
Review Comment:
[P1] Bind the physical sort and file sort-order id to one generation
This reads the sort order before `planWrite()` calls `beginWrite()`, while
opening the Iceberg transaction refreshes the same table operations. A
concurrent sort-order-only commit is not rejected by
`validateBoundWriteColumns()`, so the BE keeps the S0 `TSortInfo` but
`IcebergWriterHelper.convertToWriterResult(transaction.table(), ...)` stamps
each DataFile with refreshed S1 via `withSortOrder()`. For example, unsorted ->
`ORDER BY b` writes unsorted files that claim order `b`; `ORDER BY a` -> `ORDER
BY b` sorts by `a` but labels the files as `b`. That metadata is an Iceberg
file contract, not just a planning hint. Please build both the BE sort and
DataFile order from the post-refresh transaction generation, or carry and
validate the exact sort-order id/signature across this boundary, with a
sort-order-only evolution test.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java:
##########
@@ -173,6 +175,7 @@ public ConnectorSinkPlan planWrite(ConnectorSession
session, ConnectorWriteHandl
IcebergWriteContext writeContext = buildWriteContext(handle);
transaction.beginWrite(session, tableHandle.getDbName(),
tableHandle.getTableName(), writeContext);
Table table = transaction.getTable();
+ validateBoundWriteColumns(table, handle,
writeContext.getWriteOperation());
Review Comment:
[P1] Fence the static overwrite partition spec too
This post-refresh check only compares `table.schema()`. If the write was
bound against identity partition field `p`, a concurrent
`updateSpec().renameField("p", "q")` leaves the source schema unchanged, so the
stale `{p=7}` spec passes after `beginWrite()` refreshes to S1. At commit,
`buildPartitionFilter()` iterates S1 field `q`, finds no matching key, and
turns the nonempty spec into `Expressions.alwaysTrue()`;
`overwriteByRowFilter(alwaysTrue())` then replaces the whole table. The BE can
still write `q` from source column `p=7`, so this need not fail earlier. Please
carry and validate the bind-time spec id/field identity here and fail loud when
any nonempty static entry is unmatched, with a
rename-between-bind-and-beginWrite test.
##########
be/src/format_v2/table_reader.h:
##########
@@ -1216,17 +1216,40 @@ class TableReader {
if (const auto* array_type = typeid_cast<const
DataTypeArray*>(table_type.get())) {
const auto& array_column = assert_cast<const
ColumnArray&>(**column);
ColumnPtr nested_column = array_column.get_data_ptr();
- RETURN_IF_ERROR(
- _align_column_nullability(&nested_column,
array_type->get_nested_type()));
+ NullMap descendant_parent_null_map;
+ // Collection entries use offset coordinates, so inherited row
masks must be projected
+ // only when a required descendant can consume them. This avoids
scratch proportional
+ // to all array entries for the common all-required schema.
+ const NullMap* descendant_parent_null_map_ptr = nullptr;
+ if (_requires_parent_null_map_for_alignment(nested_column,
Review Comment:
[P1] The new gate still projects when a hidden row owns no entries
The current head fixes the null/all-clear parent-mask case, but a set
row-domain bit still does not mean the mask can hide an entry. For parent mask
`[1, 0]` and ARRAY offsets `[0, N]`, the only hidden row has an empty array. If
the visible row contains a large nullable STRUCT element stream with one
required-child physical NULL already covered by the element's own null bit,
this preflight scans those N entries, `_project_collection_parent_null_map()`
allocates and zero-fills an N-entry mask solely because row 0 is hidden, and
the nullable-element recursion allocates a second N-entry union with that
all-zero mask. MAP values have the same shape. Please first establish that a
hidden row covers a non-empty `[begin,end)` offset range before scanning
descendants/projecting, and add variable-cardinality ARRAY/MAP coverage for an
empty hidden row plus a large visible collection.
--
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]
