github-actions[bot] commented on code in PR #68640: URL: https://github.com/apache/doris/pull/68640#discussion_r4134022340
########## regression-test/suites/external_table_p0/iceberg/test_iceberg_show_nullable.groovy: ########## @@ -0,0 +1,70 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_iceberg_show_nullable", "p0,external") { + String enabled = context.config.otherConfigs.get("enableIcebergTest") + if (enabled == null || !enabled.equalsIgnoreCase("true")) { + logger.info("disable iceberg test.") + return + } + + String rest_port = context.config.otherConfigs.get("iceberg_rest_uri_port") + String minio_port = context.config.otherConfigs.get("iceberg_minio_port") + String externalEnvIp = context.config.otherConfigs.get("externalEnvIp") + String catalog_name = "test_iceberg_show_nullable" + + sql """drop catalog if exists ${catalog_name}""" + sql """ + CREATE CATALOG ${catalog_name} PROPERTIES ( + 'type'='iceberg', + 'iceberg.catalog.type'='rest', + 'uri' = 'http://${externalEnvIp}:${rest_port}', + "s3.access_key" = "admin", + "s3.secret_key" = "password", + "s3.endpoint" = "http://${externalEnvIp}:${minio_port}";, + "s3.region" = "us-east-1" + );""" + + sql """switch ${catalog_name}""" + sql """drop database if exists test_iceberg_show_nullable_db force""" + sql """create database test_iceberg_show_nullable_db""" + sql """use test_iceberg_show_nullable_db""" + + sql """ + create table iceberg_show_nullable ( + id bigint not null, + value string not null, + event_time datetime + )""" + sql """insert into iceberg_show_nullable values (1, 'a', '2026-01-01 00:00:00'), (2, 'b', null)""" + + qt_desc """desc iceberg_show_nullable""" + qt_show_columns """show columns from iceberg_show_nullable""" + // SHOW COLUMNS ... WHERE is rewritten to information_schema.columns. + qt_show_columns_where """show columns from iceberg_show_nullable where `Null` = 'NO'""" Review Comment: [P3] Use the required ordered assertion for this two-row snapshot. `SHOW COLUMNS ... WHERE` becomes a `SELECT` from `information_schema.columns` without `ORDER BY`, while this `qt_` case records `id` then `value` as a fixed sequence. The current scanner emits schema order, but the query does not request that order, and the regression standard requires `order_qt` or explicit `ORDER BY` for multirow results. Use `order_qt_show_columns_where` here. ########## fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorColumn.java: ########## @@ -179,7 +186,16 @@ public ConnectorColumn withUniqueId(int uniqueId) { public ConnectorColumn withDefaultValueSql(String defaultValueSql) { return new ConnectorColumn(name, type, comment, nullable, defaultValue, isKey, isAutoInc, isAggregated, withTimeZone, visible, uniqueId, reservedPassthrough, - nullableSpecified, commentSpecified, defaultValueSql); + nullableSpecified, commentSpecified, defaultValueSql, declaredNullable); + } + + /** + * Returns a copy carrying the nullability the source schema declares. See {@link #isDeclaredNullable()}. + */ + public ConnectorColumn withDeclaredNullable(boolean declaredNullable) { Review Comment: [P1] Bump the connector plugin API major for this new SPI call. An Iceberg plugin built from this change invokes `ConnectorColumn.withDeclaredNullable()`, but its manifest still declares API 11.0. An older FE with API 11.0 accepts that plugin, loads `ConnectorColumn` from its own parent-first SPI jar, and then throws `NoSuchMethodError` when it loads an Iceberg schema. Increment the connector API major (and update the version pin) so the gate rejects that incompatible pairing. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java: ########## @@ -854,6 +858,38 @@ public List<Column> getFullSchema() { return appendSyntheticWriteColumns(super.getFullSchema()); } + @Override + public List<Column> getBaseSchemaForDisplay() { + return toDisplaySchema(getBaseSchema()); + } + + @Override + public List<Column> getBaseSchemaForDisplay(boolean full) { + return toDisplaySchema(getBaseSchema(full)); + } + + /** + * Applies the NOT NULL the connector declared (ConnectorColumn.isDeclaredNullable()) to copies of the + * columns. The cached columns keep their nullable read semantics, which scans and writes rely on. + */ + private List<Column> toDisplaySchema(List<Column> schema) { + Set<String> declaredNotNullColumns = getSchemaCacheValue() + .map(value -> ((PluginDrivenSchemaCacheValue) value).getDeclaredNotNullColumns()) + .orElse(Collections.emptySet()); + if (declaredNotNullColumns.isEmpty()) { + return schema; + } + List<Column> displaySchema = new ArrayList<>(schema.size()); + for (Column column : schema) { + if (declaredNotNullColumns.contains(column.getName())) { + column = new Column(column); Review Comment: [P2] Preserve the timezone marker when copying required columns. For a required Iceberg `TIMESTAMP WITH TIME ZONE` field, `parseSchema()` and `ConnectorColumnConverter` put `WITH_TIMEZONE` in `Column.extraInfo`, but `new Column(column)` does not copy that field. `DESC` now receives this display copy and silently loses the existing Extra marker. Use a copy that retains `extraInfo` and cover a required zoned timestamp in the display test. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java: ########## @@ -854,6 +858,38 @@ public List<Column> getFullSchema() { return appendSyntheticWriteColumns(super.getFullSchema()); } + @Override + public List<Column> getBaseSchemaForDisplay() { + return toDisplaySchema(getBaseSchema()); + } + + @Override + public List<Column> getBaseSchemaForDisplay(boolean full) { + return toDisplaySchema(getBaseSchema(full)); + } + + /** + * Applies the NOT NULL the connector declared (ConnectorColumn.isDeclaredNullable()) to copies of the + * columns. The cached columns keep their nullable read semantics, which scans and writes rely on. + */ + private List<Column> toDisplaySchema(List<Column> schema) { + Set<String> declaredNotNullColumns = getSchemaCacheValue() Review Comment: [P2] Use one schema generation for columns and declared nullability. `getBaseSchema()` has already acquired a schema value, but this second `getSchemaCacheValue()` can fetch another one. With `meta.cache.iceberg.table.ttl-second=0`, a single `DESC` or `SHOW CREATE` loads the remote Iceberg table twice; if its schema changes between loads, the displayed type can come from the old schema and the `NULL` flag from the new one. Apply the flags from the same cache value that supplied the columns. ########## fe/fe-core/src/main/java/org/apache/doris/catalog/TableIf.java: ########## @@ -175,6 +175,20 @@ default List<Column> getBaseSchemaOrEmpty() { List<Column> getBaseSchema(boolean full); + /** + * The base schema shown by metadata statements (DESC, SHOW CREATE TABLE, SHOW COLUMNS and + * information_schema.columns). It differs from {@link #getBaseSchema()} only for a table whose source + * declares columns NOT NULL that are still read as nullable. + */ + default List<Column> getBaseSchemaForDisplay() { Review Comment: [P2] Apply declared nullability to the public schema REST response too. `GET /api/{catalog}/{db}/{table}/_schema` accepts external tables but still calls `getBaseSchema()` and renders its `is_nullable` value. For the same required Iceberg `id`, this endpoint returns `Yes` while `DESC`, `SHOW COLUMNS`, and `information_schema.columns` now return `No`. Have the human-readable REST schema route use the display schema as well. -- 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]
