spark-test commented on code in PR #60100:
URL: https://github.com/apache/spark/pull/60100#discussion_r4475019733
##########
sql/core/src/test/resources/sql-tests/inputs/create-table.sql:
##########
@@ -0,0 +1,90 @@
+--SET spark.sql.sources.default=parquet
Review Comment:
**Nit (P3):** `parquet` is already the built-in default of
`spark.sql.sources.default` (`SQLConf.scala:2593`), and none of the golden-test
harnesses that run this file override it. So the case under `-- Omitting USING
selects the configured default provider.` expects `USING parquet` whether or
not `CREATE TABLE` honors the configured value: a regression that ignored the
setting and always fell back to parquet would still match the golden.
Every other statement in the file names `USING` explicitly, so switching
this line to another built-in format such as `json`, then regenerating the
golden files, would let the case actually check the configured provider.
**Verification:**
- **Regression:** The default-provider case must fail if CREATE TABLE
without USING ignores spark.sql.sources.default and falls back to parquet: with
the configured non-default provider, its expected SHOW CREATE TABLE output must
name that provider rather than parquet.
- **Behavior:** Regenerating both create-table.sql golden families must
change only the default-provider case's SHOW CREATE TABLE output, and the
regenerated execution and analyzer goldens must pass their comparisons.
##########
sql/core/src/test/resources/sql-tests/inputs/create-table.sql:
##########
@@ -0,0 +1,90 @@
+--SET spark.sql.sources.default=parquet
+--SET
spark.sql.catalog.testcat=org.apache.spark.sql.connector.catalog.InMemoryTableCatalog
+
+-- CREATE TABLE behavior in the session catalog and a V2 catalog.
+CREATE NAMESPACE testcat.ns;
+CREATE TABLE ct_v1 (c INT, s STRING COMMENT 'x') USING parquet;
+CREATE TABLE testcat.ns.ct_v2 (c INT, s STRING COMMENT 'x') USING parquet;
+
+-- Omitting USING selects the configured default provider.
+CREATE TABLE ct_default_provider (c INT);
+SHOW CREATE TABLE ct_default_provider;
+
+-- IF NOT EXISTS preserves the existing schema, even when a different schema
is supplied.
+CREATE TABLE IF NOT EXISTS ct_v1 (c INT) USING parquet;
+CREATE TABLE IF NOT EXISTS testcat.ns.ct_v2 (c INT) USING parquet;
+DESCRIBE ct_v1;
+DESCRIBE testcat.ns.ct_v2;
+
+-- Creating an existing table without IF NOT EXISTS fails in both catalogs.
+CREATE TABLE ct_v1 (c INT) USING parquet;
+CREATE TABLE testcat.ns.ct_v2 (c INT) USING parquet;
+
+-- Duplicate column names, including case variants.
+CREATE TABLE ct_dup (a INT, a STRING) USING parquet;
+CREATE TABLE ct_dup (a INT, A STRING) USING parquet;
+
+-- Nested types and nullability.
+CREATE TABLE ct_nested (
+ st STRUCT<b: INT, c: STRING>,
+ m MAP<STRING, INT>,
+ arr ARRAY<INT>,
+ arr_st ARRAY<STRUCT<x: INT, y: STRING>>
+) USING parquet;
+DESCRIBE ct_nested;
+CREATE TABLE ct_not_null (c INT NOT NULL, s STRING) USING parquet;
+SHOW CREATE TABLE ct_not_null;
+
+-- Day-time interval columns in V1 and V2 tables.
+CREATE TABLE ct_interval (i INTERVAL DAY) USING parquet;
+CREATE TABLE testcat.ns.ct_interval (i INTERVAL DAY) USING parquet;
+DESCRIBE ct_interval;
+DESCRIBE testcat.ns.ct_interval;
+
+-- Qualified names: a valid schema and a missing schema.
+CREATE TABLE default.ct_qualified (c INT) USING parquet;
+CREATE TABLE ct_no_such_schema.ct_x (c INT) USING parquet;
+
+-- A named parameter supplies the CREATE TABLE identifier in dynamic SQL.
+EXECUTE IMMEDIATE 'CREATE TABLE IDENTIFIER(:n) (c INT) USING parquet' USING
'ct_exec' AS n;
+DESCRIBE ct_exec;
+
+-- Unknown data source provider and reserved table property.
+CREATE TABLE ct_bad_provider (c INT) USING no_such_provider;
+CREATE TABLE ct_reserved (c INT) USING parquet TBLPROPERTIES ('provider' =
'parquet');
+
+-- A table without an explicit collation inherits its schema's default
collation.
+CREATE SCHEMA ct_coll_schema DEFAULT COLLATION UTF8_LCASE;
+CREATE TABLE ct_coll_schema.ct_coll (s STRING) USING parquet;
+SHOW CREATE TABLE ct_coll_schema.ct_coll;
+
+-- REPLACE requires an existing V2 table; V1 tables do not support replacement.
Review Comment:
**Nit (P3):** The session-catalog failures recorded under this comment and
the one at line 68 do not depend on the target table being V1 or missing.
`ResolveSessionCatalog` rejects a session-catalog `REPLACE TABLE` or `CREATE OR
REPLACE TABLE` whenever the statement's provider is not a V2 table provider
(parquet is not one), without checking whether the table exists
(`ResolveSessionCatalog.scala:329-337`). That is why `ct_repl_missing` and
`ct_cor_missing` fail with `UNSUPPORTED_FEATURE.TABLE_OPERATION` rather than a
not-found error, and why a session-catalog replacement that names a V2 provider
is allowed. Suggested wording here:
```suggestion
-- In the session catalog, REPLACE with a non-V2 provider such as parquet is
unsupported, even
-- when the table is missing. In a V2 catalog, REPLACE requires an existing
table.
```
The line-68 comment could likewise say that the session-catalog statements
fail for the same reason, while in a V2 catalog `CREATE OR REPLACE TABLE`
creates a missing table or replaces an existing one.
**Verification:**
- **Inspection:** The reworded comments must match the recorded outcomes.
Every session-catalog parquet REPLACE TABLE and CREATE OR REPLACE TABLE fails
with UNSUPPORTED_FEATURE.TABLE_OPERATION, including those whose target does not
exist. REPLACE TABLE of a missing V2 table fails with TABLE_OR_VIEW_NOT_FOUND.
The V2 replace and create-or-replace cases succeed.
- **Behavior:** Both create-table.sql golden families must still pass with
no expected-output change attributable to these comment edits.
##########
sql/core/src/test/resources/sql-tests/inputs/create-table.sql:
##########
@@ -0,0 +1,90 @@
+--SET spark.sql.sources.default=parquet
+--SET
spark.sql.catalog.testcat=org.apache.spark.sql.connector.catalog.InMemoryTableCatalog
+
+-- CREATE TABLE behavior in the session catalog and a V2 catalog.
+CREATE NAMESPACE testcat.ns;
+CREATE TABLE ct_v1 (c INT, s STRING COMMENT 'x') USING parquet;
+CREATE TABLE testcat.ns.ct_v2 (c INT, s STRING COMMENT 'x') USING parquet;
+
+-- Omitting USING selects the configured default provider.
+CREATE TABLE ct_default_provider (c INT);
+SHOW CREATE TABLE ct_default_provider;
+
+-- IF NOT EXISTS preserves the existing schema, even when a different schema
is supplied.
+CREATE TABLE IF NOT EXISTS ct_v1 (c INT) USING parquet;
+CREATE TABLE IF NOT EXISTS testcat.ns.ct_v2 (c INT) USING parquet;
+DESCRIBE ct_v1;
+DESCRIBE testcat.ns.ct_v2;
+
+-- Creating an existing table without IF NOT EXISTS fails in both catalogs.
+CREATE TABLE ct_v1 (c INT) USING parquet;
+CREATE TABLE testcat.ns.ct_v2 (c INT) USING parquet;
+
+-- Duplicate column names, including case variants.
+CREATE TABLE ct_dup (a INT, a STRING) USING parquet;
+CREATE TABLE ct_dup (a INT, A STRING) USING parquet;
+
+-- Nested types and nullability.
+CREATE TABLE ct_nested (
+ st STRUCT<b: INT, c: STRING>,
+ m MAP<STRING, INT>,
+ arr ARRAY<INT>,
+ arr_st ARRAY<STRUCT<x: INT, y: STRING>>
+) USING parquet;
+DESCRIBE ct_nested;
+CREATE TABLE ct_not_null (c INT NOT NULL, s STRING) USING parquet;
Review Comment:
**Nit (P3):** The golden records `SHOW CREATE TABLE ct_not_null` rendering
this column as `c INT`. That matches the current default: for V1 file-source
tables, `CreateDataSourceTableCommand` stores the schema as nullable unless
`spark.sql.fileSource.insert.enforceNotNull` is true, and that setting defaults
to false (`createDataSourceTables.scala:84-85`, `DataSource.scala:463`).
However, this is the file's only `NOT NULL` case, it sits under a nullability
heading, and nothing in the input says the drop is intended, so a reader cannot
tell the expectation from an accepted bug. A short note would make it explicit,
for example:
```suggestion
-- V1 file-source tables keep NOT NULL only when
spark.sql.fileSource.insert.enforceNotNull=true,
-- so with the default (false) SHOW CREATE TABLE renders the declared c INT
NOT NULL as c INT.
CREATE TABLE ct_not_null (c INT NOT NULL, s STRING) USING parquet;
```
**Verification:**
- **Inspection:** The comment that introduces the ct_not_null case must name
the default V1 file-source nullable-schema behavior and the config that
controls it, consistent with the recorded SHOW CREATE TABLE output of c INT.
- **Behavior:** Both create-table.sql golden families must still pass with
no expected-output change attributable to this comment edit.
--
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]
