szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3141609537
##########
sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectDatabaseMetaDataSuite.scala:
##########
@@ -210,7 +210,7 @@ class SparkConnectDatabaseMetaDataSuite extends
ConnectFunSuite with RemoteSpark
val metadata = conn.getMetaData
// scalastyle:off line.size.limit
// CURRENT_PATH is excluded: getSQLKeywords drops SQL:2003 reserved
words (see companion).
- assert(metadata.getSQLKeywords ===
"ADD,AFTER,AGGREGATE,ALWAYS,ANALYZE,ANTI,ANY_VALUE,ARCHIVE,ASC,BINDING,BUCKET,BUCKETS,BYTE,CACHE,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,CURRENT_DATABASE,CURRENT_SCHEMA,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,DEFAULT_PATH,DEFINED,DEFINER,DELAY,DELIMITED,DESC,DFS,DIRECTORIES,DIRECTORY,DISTRIBUTE,DIV,DO,ELSEIF,ENFORCED,ESCAPED,EVOLUTION,EXCHANGE,EXCLUDE,EXCLUSIVE,EXIT,EXPLAIN,EXPORT,EXTEND,EXTENDED,FIELDS,FILEFORMAT,FIRST,FLOW,FOLLOWING,FORMAT,FORMATTED,FOUND,FUNCTIONS,GENERATED,GEOGRAPHY,GEOMETRY,HANDLER,HOURS,IDENTIFIED,IDENTIFIER,IF,IGNORE,ILIKE,IMMEDIATE,INCLUDE,INCLUSIVE,INCREMENT,INDEX,INDEXES,INPATH,INPUT,INPUTFORMAT,INVOKER,ITEMS,ITERATE,JSON,KEY,KEYS,LAST,LAZY,LEAVE,LEVEL,LIMIT,LINES,LIST,LOAD,LOCATION,LOCK,LOCKS,LOGICAL,LONG,LOOP,MACR
O,MAP,MATCHED,MATERIALIZED,MEASURE,METRICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PATH,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,QUALIFY,QUARTER,QUERY,RECORDREADER,RECORDWRITER,RECOVER,RECURSION,REDUCE,REFRESH,RELY,RENAME,REPAIR,REPEAT,REPEATABLE,REPLACE,RESET,RESPECT,RESTRICT,ROLE,ROLES,SCHEMA,SCHEMAS,SECONDS,SECURITY,SEMI,SEPARATED,SERDE,SERDEPROPERTIES,SETS,SHORT,SHOW,SINGLE,SKEWED,SORT,SORTED,SOURCE,STATISTICS,STORED,STRATIFY,STREAM,STREAMING,STRING,STRUCT,SUBSTR,SYNC,SYSTEM_PATH,SYSTEM_TIME,SYSTEM_VERSION,TABLES,TARGET,TBLPROPERTIES,TERMINATED,TIMEDIFF,TIMESTAMPADD,TIMESTAMPDIFF,TIMESTAMP_LTZ,TIMESTAMP_NTZ,TINYINT,TOUCH,TRANSACTION,TRANSACTIONS,TRANSFORM,TRUNCATE,TRY_CAST,TYPE,UNARCHIVE,UNBOUNDED,UNCACHE,UNLOCK,UNPIVOT,UNSET,UNTIL,USE,VAR,VARIABLE,VARIANT,VERSION,VIEW,VIEWS,VOID,WATERM
ARK,WEEK,WEEKS,WHILE,X,YEARS,ZONE")
+ assert(metadata.getSQLKeywords ===
"ADD,AFTER,AGGREGATE,ALWAYS,ANALYZE,ANTI,ANY_VALUE,ARCHIVE,ASC,BERNOULLI,BINDING,BUCKET,BUCKETS,BYTE,CACHE,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,CURRENT_DATABASE,CURRENT_SCHEMA,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,DEFAULT_PATH,DEFINED,DEFINER,DELAY,DELIMITED,DESC,DFS,DIRECTORIES,DIRECTORY,DISTRIBUTE,DIV,DO,ELSEIF,ENFORCED,ESCAPED,EVOLUTION,EXCHANGE,EXCLUDE,EXCLUSIVE,EXIT,EXPLAIN,EXPORT,EXTEND,EXTENDED,FIELDS,FILEFORMAT,FIRST,FLOW,FOLLOWING,FORMAT,FORMATTED,FOUND,FUNCTIONS,GENERATED,GEOGRAPHY,GEOMETRY,HANDLER,HOURS,IDENTIFIED,IDENTIFIER,IF,IGNORE,ILIKE,IMMEDIATE,INCLUDE,INCLUSIVE,INCREMENT,INDEX,INDEXES,INPATH,INPUT,INPUTFORMAT,INVOKER,ITEMS,ITERATE,JSON,KEY,KEYS,LAST,LAZY,LEAVE,LEVEL,LIMIT,LINES,LIST,LOAD,LOCATION,LOCK,LOCKS,LOGICAL,LONG
,LOOP,MACRO,MAP,MATCHED,MATERIALIZED,MEASURE,METRICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PATH,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,QUALIFY,QUARTER,QUERY,RECORDREADER,RECORDWRITER,RECOVER,RECURSION,REDUCE,REFRESH,RELY,RENAME,REPAIR,REPEAT,REPEATABLE,REPLACE,RESET,RESPECT,RESTRICT,ROLE,ROLES,SCHEMA,SCHEMAS,SECONDS,SECURITY,SEMI,SEPARATED,SERDE,SERDEPROPERTIES,SETS,SHORT,SHOW,SINGLE,SKEWED,SORT,SORTED,SOURCE,STATISTICS,STORED,STRATIFY,STREAM,STREAMING,STRING,STRUCT,SUBSTR,SYNC,SYSTEM_PATH,SYSTEM_TIME,SYSTEM_VERSION,TABLES,TARGET,TBLPROPERTIES,TERMINATED,TIMEDIFF,TIMESTAMPADD,TIMESTAMPDIFF,TIMESTAMP_LTZ,TIMESTAMP_NTZ,TINYINT,TOUCH,TRANSACTION,TRANSACTIONS,TRANSFORM,TRUNCATE,TRY_CAST,TYPE,UNARCHIVE,UNBOUNDED,UNCACHE,UNLOCK,UNPIVOT,UNSET,UNTIL,USE,VAR,VARIABLE,VARIANT,VERSION,VIEW,VIEWS,V
OID,WATERMARK,WEEK,WEEKS,WHILE,X,YEARS,ZONE")
Review Comment:
**Keywords:** `BERNOULLI` was added to the expected `getSQLKeywords` string,
but the substring still runs `SYNC,SYSTEM_PATH,...` (no standalone `SYSTEM`),
while `ThriftServerWithSparkContextSuite` includes both `BERNOULLI` and
`SYSTEM` in `CLI_ODBC_KEYWORDS`. If Connect intentionally omits `SYSTEM` (e.g.
SQL:2003 reserved-word filtering per the comment above), a brief note here
would help future contributors avoid “fixing” it incorrectly.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -885,6 +885,207 @@ class PlanParserSuite extends AnalysisTest {
stop = 65))
}
+ test("SPARK-55978: TABLESAMPLE SYSTEM and BERNOULLI - basic parsing") {
+ val sql = "select * from t"
+ // SYSTEM produces SampleMethod.System
+ assertEqual(
+ s"$sql tablesample system (43 percent) as x",
+ Sample(0, .43d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ // BERNOULLI produces SampleMethod.Bernoulli
+ assertEqual(
+ s"$sql tablesample bernoulli (43 percent) as x",
+ Sample(0, .43d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.Bernoulli).select(star()))
+ // No qualifier defaults to Bernoulli (backward compat)
+ assertEqual(
+ s"$sql tablesample(43 percent) as x",
+ Sample(0, .43d, withReplacement = false, None,
+ table("t").as("x")).select(star()))
+ }
+
+ test("SPARK-55978: TABLESAMPLE SYSTEM - case insensitivity") {
+ val sql = "select * from t"
+ // Keywords are case-insensitive
+ assertEqual(
+ s"$sql TABLESAMPLE SYSTEM (43 PERCENT) as x",
+ Sample(0, .43d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ assertEqual(
+ s"$sql TabLeSaMpLe SyStEm (43 PeRcEnT) as x",
+ Sample(0, .43d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ assertEqual(
+ s"$sql TABLESAMPLE BERNOULLI (43 PERCENT) as x",
+ Sample(0, .43d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.Bernoulli).select(star()))
+ }
+
+ test("SPARK-55978: TABLESAMPLE SYSTEM - boundary fractions") {
+ val sql = "select * from t"
+ // 0 PERCENT
+ assertEqual(
+ s"$sql tablesample system (0 percent) as x",
+ Sample(0, 0d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ // 100 PERCENT
+ assertEqual(
+ s"$sql tablesample system (100 percent) as x",
+ Sample(0, 1d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ // Fractional percent
+ assertEqual(
+ s"$sql tablesample system (0.1 percent) as x",
+ Sample(0, 0.001d, withReplacement = false, None,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ }
+
+ test("SPARK-55978: TABLESAMPLE SYSTEM - unsupported sample methods") {
+ val sql = "select * from t"
+ // SYSTEM + ROWS -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (100 rows)"),
+ condition = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_SAMPLE_METHOD",
+ sqlState = "0A000",
+ parameters = Map("sampleMethod" -> "ROWS"),
+ context = ExpectedContext(
+ fragment = "tablesample system (100 rows)",
+ start = 16,
+ stop = 44))
+ // SYSTEM + BYTES -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (300M)"),
+ condition = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_SAMPLE_METHOD",
+ sqlState = "0A000",
+ parameters = Map("sampleMethod" -> "BYTES"),
+ context = ExpectedContext(
+ fragment = "tablesample system (300M)",
+ start = 16,
+ stop = 40))
+ // SYSTEM + BUCKET -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (bucket 4 out of
10)"),
+ condition = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_SAMPLE_METHOD",
+ sqlState = "0A000",
+ parameters = Map("sampleMethod" -> "BUCKET"),
+ context = ExpectedContext(
+ fragment = "tablesample system (bucket 4 out of 10)",
+ start = 16,
+ stop = 54))
+ // SYSTEM + BUCKET ON colname -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (bucket 4 out of 10
on x)"),
+ condition = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_SAMPLE_METHOD",
+ sqlState = "0A000",
+ parameters = Map("sampleMethod" -> "BUCKET"),
+ context = ExpectedContext(
+ fragment = "tablesample system (bucket 4 out of 10 on x)",
+ start = 16,
+ stop = 59))
+ // SYSTEM + BUCKET ON function -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (bucket 3 out of 32
on rand())"),
+ condition = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_SAMPLE_METHOD",
+ sqlState = "0A000",
+ parameters = Map("sampleMethod" -> "BUCKET"),
+ context = ExpectedContext(
+ fragment = "tablesample system (bucket 3 out of 32 on rand())",
+ start = 16,
+ stop = 64))
+ }
+
+ test("SPARK-55978: TABLESAMPLE BERNOULLI - REPEATABLE is supported") {
+ assertEqual(
+ "select * from t tablesample bernoulli (43 percent) repeatable (123) as
x",
+ Sample(0, .43d, withReplacement = false, 123L,
+ table("t").as("x"), SampleMethod.Bernoulli).select(star()))
+ }
+
+ test("SPARK-55978: TABLESAMPLE SYSTEM - REPEATABLE not supported") {
+ val sql = "select * from t"
+ checkError(
+ exception = parseException(s"$sql tablesample system (43 percent)
repeatable (123)"),
+ condition = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_REPEATABLE",
+ sqlState = "0A000",
+ context = ExpectedContext(
+ fragment = "tablesample system (43 percent) repeatable (123)",
+ start = 16,
+ stop = 63))
+ }
+
+ test("SPARK-55978: TABLESAMPLE SYSTEM - fraction out of range") {
+ val sql = "select * from t"
+ // > 100 PERCENT
+ checkError(
+ exception = parseException(s"$sql tablesample system (150 percent) as
x"),
+ condition = "_LEGACY_ERROR_TEMP_0064",
Review Comment:
**Legacy error class:** This test still expects `_LEGACY_ERROR_TEMP_0064`
for invalid SYSTEM percentages. Consider migrating to a stable
`UNSUPPORTED_FEATURE` / `INVALID_*` error class for consistency with the other
new `TABLESAMPLE_SYSTEM*` conditions.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2870,7 +2891,7 @@ class AstBuilder extends DataTypeAstBuilder
// inline table comes in two styles:
// style 1: values (1), (2), (3) -- multiple columns are supported
// style 2: values 1, 2, 3 -- only a single column is supported here
- // Strip Alias wrappers from row values — CreateStruct.apply preserves
them for
+ // Strip Alias wrappers from row values - CreateStruct.apply preserves
them for
Review Comment:
**Unrelated change:** The em dash → hyphen change in this comment is
unrelated to TABLESAMPLE; harmless but slightly widens the diff.
##########
docs/sql-ref-ansi-compliance.md:
##########
@@ -760,6 +761,7 @@ Below is a list of all the keywords in Spark SQL.
|SUBSTR|non-reserved|non-reserved|non-reserved|
|SUBSTRING|non-reserved|non-reserved|non-reserved|
|SYNC|non-reserved|non-reserved|non-reserved|
+|SYSTEM|non-reserved|non-reserved|reserved|
Review Comment:
**Keyword table:** The new `SYSTEM` row shows `reserved` in the last column
while `BERNOULLI` is `non-reserved` across the board. Worth double-checking
this row matches the same convention as neighboring keywords so the table stays
internally consistent.
--
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]
