github-actions[bot] commented on code in PR #66413:
URL: https://github.com/apache/doris/pull/66413#discussion_r3790829112
##########
regression-test/suites/external_table_p0/remote_doris/test_remote_doris_variant_select.groovy:
##########
@@ -111,8 +111,9 @@ suite("test_remote_doris_variant_select", "p0,external") {
sql """
select * from
`${catalog_arrow_name}`.`${db_name}`.`test_remote_doris_variant_select_t` order
by id
"""
- // check exception message contains
- exception "[NOT_IMPLEMENTED_ERROR]read_column_from_arrow with type
variant"
+ // Keep the concatenation inside one DSL argument; a leading '+'
starts a unary expression.
+ exception("External Variant is supported only for Parquet files in
FileScannerV2; "
Review Comment:
[P1] This expectation cannot match the current production path. The new
non-Parquet Variant gate is Iceberg-specific; FileScannerV2 still creates
RemoteDorisReader for ARROW, whose _materialize_arrow_column() returns
DataTypeVariantV2SerDe::read_column_from_arrow() directly
("read_column_from_arrow with type ..."). No production code emits this
expected text, so this regression fails deterministically. Add the intended
generic pre-reader gate for ARROW, or assert the actual Remote Doris error if
that behavior is intentional.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/scan/PluginDrivenScanNode.java:
##########
@@ -194,6 +205,87 @@ public PluginDrivenScanNode(PlanNodeId id, TupleDescriptor
desc,
this.currentHandle = tableHandle;
}
+ @Override
+ protected void doInitialize() throws UserException {
+ super.doInitialize();
+ // Compatibility must inspect the snapshot-specific handle: latest
metadata may answer
+ // COUNT(*) while an older time-travel snapshot still requires a
Variant data scan.
+ pinMvccSnapshot();
Review Comment:
[P1] Run this compatibility check after scan-slot pruning. At this point the
tuple still contains the full scan output: the project visitor calls
updateScanSlotsMaterialization() only after init(), and doFinalize() below
explicitly documents that lifecycle. As a result, SELECT id from a table with
an unused ConnectorComputeVariantType column is rejected on
execution-v11/smooth-upgrade backends even though no Variant payload is read;
COUNT plans can also be falsely deferred and lose streaming. Keep the snapshot
pin here, but derive projectsComputeVariant() from the finalized tuple (or
re-run the decision in finalization), and cover the real init -> prune
lifecycle.
##########
be/src/format_v2/table/iceberg_reader.cpp:
##########
@@ -64,6 +64,94 @@ namespace doris::format::iceberg {
static constexpr const char* ROW_LINEAGE_ROW_ID = "_row_id";
static constexpr int32_t ROW_LINEAGE_ROW_ID_FIELD_ID = 2147483540;
+namespace {
+
+bool contains_variant_type(const DataTypePtr& input) {
+ if (input == nullptr) {
+ return false;
+ }
+ const auto type = remove_nullable(input);
+ switch (type->get_primitive_type()) {
+ case TYPE_VARIANT:
+ return true;
+ case TYPE_ARRAY:
+ return contains_variant_type(assert_cast<const
DataTypeArray&>(*type).get_nested_type());
+ case TYPE_MAP: {
+ const auto& map = assert_cast<const DataTypeMap&>(*type);
+ return contains_variant_type(map.get_key_type()) ||
+ contains_variant_type(map.get_value_type());
+ }
+ case TYPE_STRUCT:
+ return std::ranges::any_of(assert_cast<const
DataTypeStruct&>(*type).get_elements(),
+ contains_variant_type);
+ default:
+ return false;
+ }
+}
+
+bool mapping_reads_variant(const format::ColumnMapping& mapping) {
+ if (!mapping.file_local_id.has_value()) {
+ return false;
+ }
+ if (contains_variant_type(mapping.original_file_type)) {
+ return true;
+ }
+ if (mapping.table_type != nullptr &&
+ remove_nullable(mapping.table_type)->get_primitive_type() ==
TYPE_VARIANT) {
+ return true;
+ }
+ return std::ranges::any_of(mapping.child_mappings, mapping_reads_variant);
+}
+
+const char* file_format_name(FileFormat format) {
+ switch (format) {
+ case FileFormat::PARQUET:
+ return "PARQUET";
+ case FileFormat::ORC:
+ return "ORC";
+ case FileFormat::CSV:
+ return "CSV";
+ case FileFormat::JSON:
+ return "JSON";
+ case FileFormat::TEXT:
+ return "TEXT";
+ case FileFormat::JNI:
+ return "JNI";
+ case FileFormat::NATIVE:
+ return "NATIVE";
+ case FileFormat::ARROW:
+ return "ARROW";
+ case FileFormat::WAL:
+ return "WAL";
+ }
+ return "UNKNOWN";
+}
+
+} // namespace
+
+Status IcebergTableReader::validate_variant_file_mappings(
+ FileFormat format, const std::vector<format::ColumnMapping>& mappings)
{
+ if (format == FileFormat::PARQUET || !std::ranges::any_of(mappings,
mapping_reads_variant)) {
+ return Status::OK();
+ }
+ // Gate on a physical mapping, not the table schema: an older ORC/Avro
file may legitimately
+ // omit a Variant field added by schema evolution, in which case the
mapper synthesizes NULL.
+ return Status::NotSupported(
+ "Iceberg Variant is supported only for Parquet files in
FileScannerV2; file format {} "
+ "(including ORC/Avro readers) is not supported",
+ file_format_name(format));
+}
+
+Status IcebergTableReader::validate_file_mapping(const
format::TableColumnMapper& mapper) const {
+ if (_push_down_agg_type == TPushAggOp::type::COUNT &&
_push_down_count_columns.has_value() &&
Review Comment:
[P1] Do not exempt this mapping unless the selected reader will actually
skip the placeholder. Position deletes or a deletion vector make
TableReader::_supports_aggregate_pushdown() fall back to a real scan. Parquet
then omits count_star_placeholder_columns and synthesizes defaults, but ORC
still puts every non-predicate column in read_columns and decodes it. Thus
COUNT(*) over an ORC file whose retained slot is a physical Variant bypasses
this gate and reaches the unsupported Variant decoder whenever deletes are
present. Teach ORC to omit/materialize COUNT(*) placeholders like Parquet, or
keep this validation active for that fallback, and cover ORC Variant + position
delete.
--
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]
