You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stanyao (via GitHub)" <gi...@apache.org> on 2026/03/24 00:29:19 UTC
[PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pu… [spark]
stanyao opened a new pull request, #54972:
URL: https://github.com/apache/spark/pull/54972
…shdown
### What changes were proposed in this pull request?
This PR adds support for ANSI SQL `TABLESAMPLE SYSTEM` (block-level sampling) alongside the existing `TABLESAMPLE BERNOULLI` (row-level sampling). Key changes:
- **SQL grammar**: Extended `TABLESAMPLE` to accept an optional `SYSTEM` or `BERNOULLI` qualifier before the sample method. Added both as non-reserved keywords.
- **Logical plan**: Introduced `SampleMethod` sealed trait (`Bernoulli`/`System`) and added it to the `Sample` node. Default is `Bernoulli` for backward compatibility.
- **Parser**: `TABLESAMPLE SYSTEM` only supports `PERCENT` sampling and does not support `REPEATABLE`. Other methods (ROWS, BUCKET, BYTES) are rejected with clear error messages.
- **DSv2 pushdown**: `TABLESAMPLE SYSTEM` is pushed down to data sources via an extended `SupportsPushDownTableSample.pushTableSample()` overload with `isSystemSampling` flag. Sources that don't override the new method reject SYSTEM sampling by default.
- **Physical planning**: SYSTEM samples that aren't pushed down to a DSv2 source raise an `AnalysisException` — there is no row-level fallback since block sampling is data-source dependent.
### Why are the changes needed?
ANSI SQL defines two sampling methods: `BERNOULLI` (row-level) and `SYSTEM` (implementation-dependent, typically block/split-level). Block sampling is significantly faster for large tables since it avoids per-row evaluation, making it useful for approximate queries and data exploration. Many databases (PostgreSQL, Hive, Trino) support this distinction.
### Does this PR introduce _any_ user-facing change?
Yes. New SQL syntax `TABLESAMPLE SYSTEM (x PERCENT)` and `TABLESAMPLE BERNOULLI (x PERCENT)`. `BERNOULLI` and `SYSTEM` are added as non-reserved keywords. Existing queries without these keywords behave identically to before.
### How was this patch tested?
- 9 new test cases in `PlanParserSuite` covering: basic parsing, case insensitivity, boundary fractions, unsupported methods (ROWS/BUCKET with SYSTEM), REPEATABLE rejection, fraction validation, identifier preservation, and subquery contexts.
- Existing `SQLQuerySuite` tests pass.
- Scalastyle passes with 0 errors/warnings.
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3025180347
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala:
##########
@@ -1039,7 +1039,18 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
execution.FilterExec(f.typedCondition(f.deserializer), planLater(f.child)) :: Nil
case e @ logical.Expand(_, _, child) =>
execution.ExpandExec(e.projections, e.output, planLater(child)) :: Nil
- case logical.Sample(lb, ub, withReplacement, seed, child) =>
+ case logical.Sample(lb, ub, withReplacement, seed, child, sampleMethod) =>
+ if (sampleMethod == logical.SampleMethod.System) {
+ // Defensive Check: TABLESAMPLE SYSTEM node wasn't able to be pushed into
+ // V2ScanRelationPushDown earlier. This means the data source is either not DSv2 or
+ // not able to support block/split sampling. Now it fell through to the row-based
+ // sampling. Therefore error out.
+ throw new AnalysisException(
Review Comment:
Added a new UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM error class.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3150302358
##########
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:
Changed it back. Will confirm lint passes. Otherwise, I'll change it to the normal dash to unblock.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3141600082
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +837,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
Review Comment:
yea i also saw that. (looked a bit at changing it for BERNOULLI) I think it may be riskier to fix it after. but understand your point on consistency. cc @cloud-fan 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3024996984
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -36,4 +36,24 @@ boolean pushTableSample(
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with sample method awareness.
+ * Data sources can override this to distinguish SYSTEM (block) from BERNOULLI (row) sampling.
+ * By default, rejects SYSTEM sampling for backward compatibility and delegates BERNOULLI to
+ * the 4-parameter version.
+ */
+ default boolean pushTableSample(
+ double lowerBound,
+ double upperBound,
+ boolean withReplacement,
+ long seed,
+ boolean isSystemSampling) {
Review Comment:
Good point. After some debate, I settled on having a Scala sealed trait in Spark core and a matching Java class in the connector layer. There is some downside in keeping both in sync, but still better than alternatives, e.g., (1) only define a Java class and reference it in Spark core. This is bad because SampleMethod is a Spark core instead of connector level concept, (2) only define in Scala and reference it in Java at the connector layer. This is bad because SampleMethod is missing from DSv2 public API and connector developers need to write weird code like if (sampleMethod == SampleMethod.System**$.MODULE$**).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3048528581
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -880,6 +880,160 @@ 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, 10L,
+ 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, 10L,
+ 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, 10L,
+ 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, 10L,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ assertEqual(
+ s"$sql TabLeSaMpLe SyStEm (43 PeRcEnT) as x",
+ Sample(0, .43d, withReplacement = false, 10L,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ assertEqual(
+ s"$sql TABLESAMPLE BERNOULLI (43 PERCENT) as x",
+ Sample(0, .43d, withReplacement = false, 10L,
+ 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, 10L,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ // 100 PERCENT
+ assertEqual(
+ s"$sql tablesample system (100 percent) as x",
+ Sample(0, 1d, withReplacement = false, 10L,
+ 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, 10L,
+ 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 = "_LEGACY_ERROR_TEMP_0035",
+ parameters = Map(
+ "message" -> "TABLESAMPLE SYSTEM only supports PERCENT sampling"),
+ context = ExpectedContext(
+ fragment = "tablesample system (100 rows)",
+ start = 16,
+ stop = 44))
+ // SYSTEM + BYTES -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (300M)"),
+ condition = "_LEGACY_ERROR_TEMP_0035",
+ parameters = Map(
+ "message" -> "TABLESAMPLE SYSTEM only supports PERCENT sampling"),
+ 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 = "_LEGACY_ERROR_TEMP_0035",
+ parameters = Map(
+ "message" -> "TABLESAMPLE SYSTEM only supports PERCENT sampling"),
+ context = ExpectedContext(
+ fragment = "tablesample system (bucket 4 out of 10)",
+ start = 16,
+ stop = 54))
+ }
+
+ test("SPARK-55978: TABLESAMPLE BERNOULLI - REPEATABLE is supported") {
+ assertEqual(
+ "select * from t tablesample bernoulli (43 percent) repeatable (123)",
+ Sample(0, .43d, withReplacement = false, 123L,
Review Comment:
Fixed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3119991581
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -885,6 +885,188 @@ 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 = "_LEGACY_ERROR_TEMP_0035",
Review Comment:
I missed this one. Didn't realize operationNotAllowed() is mapped to _LEGACY_ERROR_TEMP_0035. Thanks for catching! Replaced with 2 new classes of errors (one for repeatable and one for non-percentage sample methods for SYSTEM). Since there is ongoing efforts to eliminate _LEGACY_ERROR_TEMP_0035, we definitely should not add more references to it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3046309190
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -812,6 +813,31 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ val sHolder = sample.child match {
+ case s: ScanBuilderHolder => s
+ case SubqueryAlias(_, s: ScanBuilderHolder) => s
Review Comment:
You are right. EliminateSubqueryAlias runs earlier than pushDownSample(). Removed this.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3048528053
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -812,6 +813,31 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ val sHolder = sample.child match {
Review Comment:
Agree. PhysicalOperation is more robust that traverse deeper down the sub plan tree to match the pattern. Fixed by using PhysicalOperation instead.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3149832511
##########
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:
Added "SYSTEM" in line 212, so it reads:
// CURRENT_PATH **and SYSTEM are** excluded: getSQLKeywords drops SQL:2003 reserved words (see companion).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3221254647
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala:
##########
@@ -1039,7 +1039,16 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
execution.FilterExec(f.typedCondition(f.deserializer), planLater(f.child)) :: Nil
case e @ logical.Expand(_, _, child) =>
execution.ExpandExec(e.projections, e.output, planLater(child)) :: Nil
- case logical.Sample(lb, ub, withReplacement, seed, child) =>
+ case logical.Sample(lb, ub, withReplacement, seed, child, sampleMethod) =>
+ if (sampleMethod == logical.SampleMethod.System) {
Review Comment:
This branch is unreachable in normal flow — `V2ScanRelationPushDown` is non-excludable and always handles SYSTEM samples (either removes the Sample node or throws). Reaching it would indicate an internal invariant violation (e.g. a custom `Optimizer` subclass that skipped `V2ScanRelationPushDown`), not a user error, so a `SparkException.internalError` would be more accurate than a user-facing `AnalysisException`.
The comment is also a bit misleading: it says "fell through to row-based sampling" but the actual cause if this is ever reached is "the SYSTEM-handling optimizer rule didn't run as expected".
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -29,11 +29,28 @@
public interface SupportsPushDownTableSample extends ScanBuilder {
/**
- * Pushes down SAMPLE to the data source.
+ * Pushes down BERNOULLI (row-level) SAMPLE to the data source.
*/
boolean pushTableSample(
double lowerBound,
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with the specified sampling method.
+ */
+ default boolean pushTableSample(
+ double lowerBound,
+ double upperBound,
+ boolean withReplacement,
+ long seed,
+ SampleMethod sampleMethod) {
+ if (sampleMethod == SampleMethod.SYSTEM) {
+ // If the data source hasn't overridden this method, it must have not added support
Review Comment:
Word order — "must not have" reads more naturally than "must have not".
```suggestion
// If the data source hasn't overridden this method, it must not have added support
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2448,10 +2448,14 @@ class AstBuilder extends DataTypeAstBuilder
* - TABLESAMPLE(x ROWS): Sample the table down to the given number of rows.
* - TABLESAMPLE(x PERCENT) [REPEATABLE (y)]: Sample the table down to the given percentage with
* seed 'y'. Note that percentages are defined as a number between 0 and 100.
+ * - TABLESAMPLE SYSTEM(x PERCENT): Sample by data source dependent blocks or file splits.
Review Comment:
Compound modifier should be hyphenated.
```suggestion
* - TABLESAMPLE SYSTEM(x PERCENT): Sample by data-source-dependent blocks or file splits.
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -838,6 +844,33 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
Review Comment:
The SYSTEM and BERNOULLI branches share most of the structure — `TableSampleInfo` construction, the `PhysicalOperation(_, Nil, sHolder)` pattern, and the `PushDownUtils.pushTableSample` call — and differ only in failure handling. Worth consolidating into a single `case sample: Sample => sample.child match { ... }` that branches on `sampleMethod` only for the throw vs. fall-back decision.
As a side effect, the two paths have already started to drift: the seed-TODO comment lives only on the SYSTEM branch even though both branches use the same `* 1000` multiplier.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3025177172
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -811,7 +812,44 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
}
+ private def findScanBuilderHolder(plan: LogicalPlan): Option[ScanBuilderHolder] = {
+ plan match {
+ case s: ScanBuilderHolder => Some(s)
+ case s: SubqueryAlias => findScanBuilderHolder(s.child)
+ case p: Project => findScanBuilderHolder(p.child)
+ case f: Filter => findScanBuilderHolder(f.child)
Review Comment:
Good point! Per the SQL standard (ISO/IEC 9075), TABLESAMPLE SYSTEM is applied to the table reference in the FROM clause, before WHERE filtering. When the FROM clause is a subquery/view with filters, it will scan the full table anyway, so block sampling won't be able to save I/O. Therefore block sampling is not meaningful in this case. Therefore, I fixed this by only matching ScanBuilderHolder or SubqueryAlias and won't push down SAMPLE through filter (or things like that). Otherwise, we will explicitly error out as unsupported feature.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3121073632
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2TableSampleSuite.scala:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.connector.catalog.InMemoryTableWithTableSampleCatalog
+
+class DataSourceV2TableSampleSuite extends DatasourceV2SQLBase
Review Comment:
Added more test cases:
- non-pushdown DSv2 catalog => UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM
- subquery => UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_NO_SCAN
- DSv1 parquet table => UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_NO_SCAN
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4426404664
thanks, merging to master/4.x/4.2!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3221686095
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -2448,10 +2448,14 @@ class AstBuilder extends DataTypeAstBuilder
* - TABLESAMPLE(x ROWS): Sample the table down to the given number of rows.
* - TABLESAMPLE(x PERCENT) [REPEATABLE (y)]: Sample the table down to the given percentage with
* seed 'y'. Note that percentages are defined as a number between 0 and 100.
+ * - TABLESAMPLE SYSTEM(x PERCENT): Sample by data source dependent blocks or file splits.
Review Comment:
Fixed.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -29,11 +29,28 @@
public interface SupportsPushDownTableSample extends ScanBuilder {
/**
- * Pushes down SAMPLE to the data source.
+ * Pushes down BERNOULLI (row-level) SAMPLE to the data source.
*/
boolean pushTableSample(
double lowerBound,
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with the specified sampling method.
+ */
+ default boolean pushTableSample(
+ double lowerBound,
+ double upperBound,
+ boolean withReplacement,
+ long seed,
+ SampleMethod sampleMethod) {
+ if (sampleMethod == SampleMethod.SYSTEM) {
+ // If the data source hasn't overridden this method, it must have not added support
Review Comment:
Fixed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3025177172
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -811,7 +812,44 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
}
+ private def findScanBuilderHolder(plan: LogicalPlan): Option[ScanBuilderHolder] = {
+ plan match {
+ case s: ScanBuilderHolder => Some(s)
+ case s: SubqueryAlias => findScanBuilderHolder(s.child)
+ case p: Project => findScanBuilderHolder(p.child)
+ case f: Filter => findScanBuilderHolder(f.child)
Review Comment:
Good point! Per the SQL standard (ISO/IEC 9075), TABLESAMPLE SYSTEM is applied to the table reference in the FROM clause, before WHERE filtering. When the FROM clause is a subquery/view with filters, it will scan the full table anyway, so block sampling won't be able to save I/O. Therefore block sampling is not meaningful in this case. Therefore, I fixed this by only matching ScanBuilderHolder or SubqueryAlias and won't push down SAMPLE through filter (or things like that). Otherwise, block sampling is not meaningful (can't save I/O), so we will explicitly error out as unsupported feature.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3090542505
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +835,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
+ sampleMethod = sample.sampleMethod)
+ val pushed = PushDownUtils.pushTableSample(sHolder.builder, tableSample)
+ if (pushed) {
+ sHolder.pushedSample = Some(tableSample)
+ sample.child
+ } else {
+ throw new AnalysisException(
+ errorClass = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM",
+ messageParameters = Map.empty)
+ }
+ case _ =>
+ throw new AnalysisException(
Review Comment:
Good catch! Fixed the "case _" with a new error message.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #54972: [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown
URL: https://github.com/apache/spark/pull/54972
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3221575712
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala:
##########
@@ -1039,7 +1039,16 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
execution.FilterExec(f.typedCondition(f.deserializer), planLater(f.child)) :: Nil
case e @ logical.Expand(_, _, child) =>
execution.ExpandExec(e.projections, e.output, planLater(child)) :: Nil
- case logical.Sample(lb, ub, withReplacement, seed, child) =>
+ case logical.Sample(lb, ub, withReplacement, seed, child, sampleMethod) =>
+ if (sampleMethod == logical.SampleMethod.System) {
Review Comment:
Very good point. Yeah, SYSTEM sample nodes should have been fully handled by V2ScanRelationPushDown (including throwing AnalysisException for user error like doing system sampling on DSV1 source); reaching this point indicates an internal error. Fixed both the error type and comment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3025179369
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -811,7 +812,44 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
}
+ private def findScanBuilderHolder(plan: LogicalPlan): Option[ScanBuilderHolder] = {
+ plan match {
+ case s: ScanBuilderHolder => Some(s)
+ case s: SubqueryAlias => findScanBuilderHolder(s.child)
+ case p: Project => findScanBuilderHolder(p.child)
+ case f: Filter => findScanBuilderHolder(f.child)
+ case _ => None
+ }
+ }
+
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ findScanBuilderHolder(sample.child) match {
+ case Some(sHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed,
+ sampleMethod = sample.sampleMethod)
+ val pushed = PushDownUtils.pushTableSample(sHolder.builder, tableSample)
+ if (pushed) {
+ sHolder.pushedSample = Some(tableSample)
+ sample.child
+ } else {
+ throw new AnalysisException(
+ errorClass = "_LEGACY_ERROR_TEMP_0035",
Review Comment:
Added a new UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM error class.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3048470893
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -29,11 +29,28 @@
public interface SupportsPushDownTableSample extends ScanBuilder {
/**
- * Pushes down SAMPLE to the data source.
+ * Pushes down BERNOULLI (row-level) SAMPLE to the data source.
*/
boolean pushTableSample(
double lowerBound,
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with the specified sampling method.
+ */
+ default boolean pushTableSample(
Review Comment:
Nice catch!! Per the SQL standard (ISO/IEC 9075-2, section 7.6), TABLESAMPLE is part of the <table reference> production, so it applies to a single table before joins are evaluated. I implemented a safe fix to honor pushDownSample and ignore pushDownJoin when both appear on the merged node, because otherwise data source could have multiple ways to interpret/handle the merged node with both sample and join push down.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3148781723
##########
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:
This is due to the call chain here -> AnalysisTest.scala -> AbstractSqlParser.scala -> AstBuilder.scala -> validate() function in ParserUtils.scala that is shared by both the existing Bernoulli sampling and the new System sampling. Fixing this will need code changes to 10 call sites and 20 test cases that are not related to this PR. It's best to be a dedicated follow up PR to keep this PR focused. I logged a JIRA item to track [SPARK-56638](https://issues.apache.org/jira/browse/SPARK-56638).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3090499997
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -150,6 +151,9 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
rightProjections.forall(_.isInstanceOf[AttributeReference]) &&
// Cross joins are not supported because they increase the amount of data.
condition.isDefined &&
+ // Do not push down join if either side has a pushed sample, as there is no
Review Comment:
Created SPARK-56504 to track this future improvement and added the todo comment here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "dbtsai (via GitHub)" <gi...@apache.org>.
dbtsai commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4137487151
cc @szehon-ho
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3089170848
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -150,6 +151,9 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
rightProjections.forall(_.isInstanceOf[AttributeReference]) &&
// Cross joins are not supported because they increase the amount of data.
condition.isDefined &&
+ // Do not push down join if either side has a pushed sample, as there is no
Review Comment:
how about put TODO. we will handle it later right?
```
// Do not push down join if either side has a pushed sample, because
// the merged scan builder would silently discard it.
// TODO: extend SupportsPushDownJoin to accept pushed samples so data
// sources that support both can handle the composition.
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +835,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
+ sampleMethod = sample.sampleMethod)
+ val pushed = PushDownUtils.pushTableSample(sHolder.builder, tableSample)
+ if (pushed) {
+ sHolder.pushedSample = Some(tableSample)
+ sample.child
+ } else {
+ throw new AnalysisException(
+ errorClass = "UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM",
+ messageParameters = Map.empty)
+ }
+ case _ =>
+ throw new AnalysisException(
Review Comment:
The error message is "TABLESAMPLE SYSTEM is only supported by data sources that implement block-level sampling." — but the actual problem here could be that the user wrote something like:
```
SELECT * FROM (SELECT * FROM t WHERE x > 5) TABLESAMPLE SYSTEM (10 PERCENT)
```
The issue isn't that the data source doesn't support block sampling — it's that there are intervening operators (subquery/filter) between SYSTEM and the table scan. Consider a more specific error message or a separate error sub-class for this case, e.g.:
```
TABLESAMPLE SYSTEM can only be applied directly to a table reference, not to a subquery or filtered result.
```
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SampleMethod.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import org.apache.spark.annotation.Evolving;
+
+/**
+ * The sampling method for TABLESAMPLE.
+ *
+ * @since 4.1.0
Review Comment:
next version is spark 4.2
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala:
##########
@@ -159,8 +159,10 @@ case class RowDataSourceScanExec(
private def seqToString(seq: Seq[Any]): String = seq.mkString("[", ", ", "]")
- private def pushedSampleMetadataString(s: TableSampleInfo): String =
- s"SAMPLE (${(s.upperBound - s.lowerBound) * 100}) ${s.withReplacement} SEED(${s.seed})"
+ private def pushedSampleMetadataString(s: TableSampleInfo): String = {
+ val pct = (s.upperBound - s.lowerBound) * 100
Review Comment:
This will output Bernoulli SAMPLE (10.0) false SEED(123) or System SAMPLE (10.0) false SEED(123). The casing (Bernoulli/System) comes from the Scala case object names. Should this be uppercase (BERNOULLI/SYSTEM) to match the SQL keywords? It's a cosmetic nit but matters for consistency in EXPLAIN output.
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -2187,6 +2190,7 @@ ansiNonReserved
| SUBSTR
| SUBSTRING
| SYNC
+ | SYSTEM
Review Comment:
im a little bit unsure about this part.
Let's add some parser tests to verify table call 'system' still works:
```
-- SYSTEM as table name with TABLESAMPLE (no qualifier)
SELECT * FROM system TABLESAMPLE(10 PERCENT) AS x
-- SYSTEM as table name with TABLESAMPLE SYSTEM qualifier
SELECT * FROM system TABLESAMPLE SYSTEM (10 PERCENT) AS x
-- SYSTEM as both table name and alias with TABLESAMPLE
SELECT * FROM system TABLESAMPLE(10 PERCENT) system
-- BERNOULLI as table name with TABLESAMPLE BERNOULLI qualifier
SELECT * FROM bernoulli TABLESAMPLE BERNOULLI (10 PERCENT) AS x
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3090438041
##########
sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectDatabaseMetaDataSuite.scala:
##########
@@ -209,7 +209,7 @@ class SparkConnectDatabaseMetaDataSuite extends ConnectFunSuite with RemoteSpark
withConnection { conn =>
val metadata = conn.getMetaData
// scalastyle:off line.size.limit
- assert(metadata.getSQLKeywords === "ADD,AFTER,AGGREGATE,ALWAYS,ANALYZE,ANTI,ANY_VALUE,ARCHIVE,ASC,BINDING,BUCKET,BUCKETS,BYTE,CACHE,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,MET
RICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,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,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,M
EASURE,METRICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,WEEK,WEEKS,WHILE,X,YEARS,ZONE")
Review Comment:
Created [SPARK-56503](https://issues.apache.org/jira/browse/SPARK-56503) to track this follow up.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3117366583
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -885,6 +885,188 @@ 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 = "_LEGACY_ERROR_TEMP_0035",
Review Comment:
just wondering, is it hard to fix these messages as well?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +837,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
Review Comment:
btw, why 1000? there's only 1000 values then, we should start with a better default seed range for SAMPLE?
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2TableSampleSuite.scala:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.connector.catalog.InMemoryTableWithTableSampleCatalog
+
+class DataSourceV2TableSampleSuite extends DatasourceV2SQLBase
Review Comment:
should we add another test to capture pushed systemtable EXPLAIN ?
To capture the changes to pushedSampleMetadataString
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3121073632
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2TableSampleSuite.scala:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.connector.catalog.InMemoryTableWithTableSampleCatalog
+
+class DataSourceV2TableSampleSuite extends DatasourceV2SQLBase
Review Comment:
Added more test cases:
- non-pushdown DSv2 catalog => error TABLESAMPLE_SYSTEM
- subquery => TABLESAMPLE_SYSTEM_NO_SCAN
- DSv1 parquet table => TABLESAMPLE_SYSTEM_NO_SCAN
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4426046804
This has been approved and CI is green. Could you please merge when ready? Thanks! /cc: @cloud-fan , @szehon-ho
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4211597158
Squashed multiple commits into a single one to make code review easier.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3048524986
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -812,6 +813,31 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ val sHolder = sample.child match {
+ case s: ScanBuilderHolder => s
+ case SubqueryAlias(_, s: ScanBuilderHolder) => s
Review Comment:
Indeed not necessary. Removed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4148464426
Rebased and resolved the new merge conflict above.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3042327504
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -812,6 +813,31 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ val sHolder = sample.child match {
+ case s: ScanBuilderHolder => s
+ case SubqueryAlias(_, s: ScanBuilderHolder) => s
Review Comment:
can you check if this is necessary? There's a rule EliminateSubqueryAlias that should do it
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/TableSampleInfo.scala:
##########
@@ -17,8 +17,11 @@
package org.apache.spark.sql.execution.datasources.v2
+import org.apache.spark.sql.catalyst.plans.logical.SampleMethod
+
case class TableSampleInfo(
lowerBound: Double,
upperBound: Double,
withReplacement: Boolean,
- seed: Long)
+ seed: Long,
+ sampleMethod: SampleMethod = SampleMethod.Bernoulli)
Review Comment:
pushedSampleMetadataString in DataSourceScanExec.scala should be updated to include the sample method in its output, so explain can distinguish between pushed BERNOULLI and SYSTEM samples.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -7968,6 +7968,11 @@
"message" : [
"The target table is <table>."
]
+ },
+ "TABLESAMPLE_SYSTEM" : {
+ "message" : [
+ "TABLESAMPLE SYSTEM (block sampling) requires a DSv2 data source that supports sample pushdown (SupportsPushDownTableSample)."
Review Comment:
how about a more user-friendly one?
```
TABLESAMPLE SYSTEM is only supported by data sources that implement block-level sampling.
```
##########
sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectDatabaseMetaDataSuite.scala:
##########
@@ -209,7 +209,7 @@ class SparkConnectDatabaseMetaDataSuite extends ConnectFunSuite with RemoteSpark
withConnection { conn =>
val metadata = conn.getMetaData
// scalastyle:off line.size.limit
- assert(metadata.getSQLKeywords === "ADD,AFTER,AGGREGATE,ALWAYS,ANALYZE,ANTI,ANY_VALUE,ARCHIVE,ASC,BINDING,BUCKET,BUCKETS,BYTE,CACHE,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,MET
RICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,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,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,M
EASURE,METRICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,WEEK,WEEKS,WHILE,X,YEARS,ZONE")
Review Comment:
by the way, this doesnt support for Spark connect yet? (proto Sample message should include sample_method)
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -29,11 +29,28 @@
public interface SupportsPushDownTableSample extends ScanBuilder {
/**
- * Pushes down SAMPLE to the data source.
+ * Pushes down BERNOULLI (row-level) SAMPLE to the data source.
*/
boolean pushTableSample(
double lowerBound,
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with the specified sampling method.
+ */
+ default boolean pushTableSample(
Review Comment:
We need to add a guard to pushdownJoin to not allow following scenario:
If a connector implements both SupportsPushdownJoin and new SupportsPushdownSample method, this happens:
After createScanBuilder:
```
Join
Sample(System)
ScanBuilderHolder(t1) ← builder accepted SYSTEM
ScanBuilderHolder(t2)
```
After pushDownSample — SYSTEM pushdown succeeds, Sample node removed:
```
Join
ScanBuilderHolder(t1) ← pushedSample = Some(System, 10%)
ScanBuilderHolder(t2)
```
After pushDownJoin — join pushdown matches (both sides are bare ScanBuilderHolder via PhysicalOperation):
```
ScanBuilderHolder(merged) ← pushedSample = None (line 215)
````
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -812,6 +813,31 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ val sHolder = sample.child match {
Review Comment:
actually, i think we can re-use the PhysicalOperation exactor like the other pushdowns?
PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder)
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala:
##########
@@ -880,6 +880,160 @@ 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, 10L,
+ 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, 10L,
+ 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, 10L,
+ 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, 10L,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ assertEqual(
+ s"$sql TabLeSaMpLe SyStEm (43 PeRcEnT) as x",
+ Sample(0, .43d, withReplacement = false, 10L,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ assertEqual(
+ s"$sql TABLESAMPLE BERNOULLI (43 PERCENT) as x",
+ Sample(0, .43d, withReplacement = false, 10L,
+ 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, 10L,
+ table("t").as("x"), SampleMethod.System).select(star()))
+ // 100 PERCENT
+ assertEqual(
+ s"$sql tablesample system (100 percent) as x",
+ Sample(0, 1d, withReplacement = false, 10L,
+ 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, 10L,
+ 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 = "_LEGACY_ERROR_TEMP_0035",
+ parameters = Map(
+ "message" -> "TABLESAMPLE SYSTEM only supports PERCENT sampling"),
+ context = ExpectedContext(
+ fragment = "tablesample system (100 rows)",
+ start = 16,
+ stop = 44))
+ // SYSTEM + BYTES -> error
+ checkError(
+ exception = parseException(s"$sql tablesample system (300M)"),
+ condition = "_LEGACY_ERROR_TEMP_0035",
+ parameters = Map(
+ "message" -> "TABLESAMPLE SYSTEM only supports PERCENT sampling"),
+ 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 = "_LEGACY_ERROR_TEMP_0035",
+ parameters = Map(
+ "message" -> "TABLESAMPLE SYSTEM only supports PERCENT sampling"),
+ context = ExpectedContext(
+ fragment = "tablesample system (bucket 4 out of 10)",
+ start = 16,
+ stop = 54))
+ }
+
+ test("SPARK-55978: TABLESAMPLE BERNOULLI - REPEATABLE is supported") {
+ assertEqual(
+ "select * from t tablesample bernoulli (43 percent) repeatable (123)",
+ Sample(0, .43d, withReplacement = false, 123L,
Review Comment:
nit: the other tests have asX
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3090596567
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -2187,6 +2190,7 @@ ansiNonReserved
| SUBSTR
| SUBSTRING
| SYNC
+ | SYSTEM
Review Comment:
Yeah, these are good test cases. Besides adding these 4, I also added 3 additional ones to cross test mix of "System" and "Bernoulli".
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3150302358
##########
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:
This actually breaks linter (NonASCIICharacterChecker enabled at scalastyle-config.xml:119). Since it is just one character in the comment, I'd recommend keeping it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3089235468
##########
sql/api/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBaseParser.g4:
##########
@@ -2187,6 +2190,7 @@ ansiNonReserved
| SUBSTR
| SUBSTRING
| SYNC
+ | SYSTEM
Review Comment:
@cloud-fan can you help check the parser changes ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3090558329
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala:
##########
@@ -159,8 +159,10 @@ case class RowDataSourceScanExec(
private def seqToString(seq: Seq[Any]): String = seq.mkString("[", ", ", "]")
- private def pushedSampleMetadataString(s: TableSampleInfo): String =
- s"SAMPLE (${(s.upperBound - s.lowerBound) * 100}) ${s.withReplacement} SEED(${s.seed})"
+ private def pushedSampleMetadataString(s: TableSampleInfo): String = {
+ val pct = (s.upperBound - s.lowerBound) * 100
Review Comment:
Nice catch. Added .toUpperCase.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3125195784
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +837,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
Review Comment:
The reason 1000 is used here is to be consistent with SampleExec.resolvedSeed in basicPhysicalOperators.scala:362. Based on community convention, I'd keep this as is to be consistent with existing patterns and keep this PR focused and open a JIRA issue to fix both places at once (issues.apache.org doesn't load for some reason, so I'll get to that later today). What do you think, @szehon-ho ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3125195784
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +837,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
Review Comment:
The reason 1000 is used here is to be consistent with SampleExec.resolvedSeed in basicPhysicalOperators.scala:362. Based on community convention, I'd keep this as is to be consistent with existing patterns and keep this PR focused and open a JIRA issue to fix both places at once ([SPARK-56573](https://issues.apache.org/jira/browse/SPARK-56573)). What do you think, @szehon-ho ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3221575712
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala:
##########
@@ -1039,7 +1039,16 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
execution.FilterExec(f.typedCondition(f.deserializer), planLater(f.child)) :: Nil
case e @ logical.Expand(_, _, child) =>
execution.ExpandExec(e.projections, e.output, planLater(child)) :: Nil
- case logical.Sample(lb, ub, withReplacement, seed, child) =>
+ case logical.Sample(lb, ub, withReplacement, seed, child, sampleMethod) =>
+ if (sampleMethod == logical.SampleMethod.System) {
Review Comment:
Very good point. Yeah, SYSTEM sample nodes should have been fully handled by V2ScanRelationPushDown (including throwing AnalysisException for user error like doing system sampling on DSV1 source); reaching this point indicates an internal error. Fixed both the error type and comment, while still keeping this defensive check.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3221683790
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -838,6 +844,33 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
Review Comment:
Good catch! The new case for SYSTEM started differently and evolved to be very similar to BERNOULLI case. I have merged them to reduce duplicate code.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3150296881
##########
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:
Double checked. The SQL-2016 column is correct for both keywords. SYSTEM is a reserved word in the SQL:2016 standard while BERNOULLI is not. The reasoning for that is that SYSTEM is reserved broadly because of its wide use in other syntax like SYSTEM_USER, SYSTEM VERSIONING, etc.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3004268923
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -36,4 +36,24 @@ boolean pushTableSample(
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with sample method awareness.
+ * Data sources can override this to distinguish SYSTEM (block) from BERNOULLI (row) sampling.
+ * By default, rejects SYSTEM sampling for backward compatibility and delegates BERNOULLI to
+ * the 4-parameter version.
+ */
+ default boolean pushTableSample(
+ double lowerBound,
+ double upperBound,
+ boolean withReplacement,
+ long seed,
+ boolean isSystemSampling) {
Review Comment:
did we consider a DSV2 SampleMethod enum (for potentially other sample methods, corresponding to the one you added in Analyzer)
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -36,4 +36,24 @@ boolean pushTableSample(
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with sample method awareness.
+ * Data sources can override this to distinguish SYSTEM (block) from BERNOULLI (row) sampling.
+ * By default, rejects SYSTEM sampling for backward compatibility and delegates BERNOULLI to
Review Comment:
again, not sure this comment is that helpful. i think it'd be more helpful to modify the javadoc of the 4-parameter one to say its BERNOULLI
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala:
##########
@@ -1039,7 +1039,18 @@ abstract class SparkStrategies extends QueryPlanner[SparkPlan] {
execution.FilterExec(f.typedCondition(f.deserializer), planLater(f.child)) :: Nil
case e @ logical.Expand(_, _, child) =>
execution.ExpandExec(e.projections, e.output, planLater(child)) :: Nil
- case logical.Sample(lb, ub, withReplacement, seed, child) =>
+ case logical.Sample(lb, ub, withReplacement, seed, child, sampleMethod) =>
+ if (sampleMethod == logical.SampleMethod.System) {
+ // Defensive Check: TABLESAMPLE SYSTEM node wasn't able to be pushed into
+ // V2ScanRelationPushDown earlier. This means the data source is either not DSv2 or
+ // not able to support block/split sampling. Now it fell through to the row-based
+ // sampling. Therefore error out.
+ throw new AnalysisException(
Review Comment:
same, should not throw Legacy error
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -811,7 +812,44 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
}
+ private def findScanBuilderHolder(plan: LogicalPlan): Option[ScanBuilderHolder] = {
+ plan match {
+ case s: ScanBuilderHolder => Some(s)
+ case s: SubqueryAlias => findScanBuilderHolder(s.child)
+ case p: Project => findScanBuilderHolder(p.child)
+ case f: Filter => findScanBuilderHolder(f.child)
+ case _ => None
+ }
+ }
+
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ findScanBuilderHolder(sample.child) match {
+ case Some(sHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed,
+ sampleMethod = sample.sampleMethod)
+ val pushed = PushDownUtils.pushTableSample(sHolder.builder, tableSample)
+ if (pushed) {
+ sHolder.pushedSample = Some(tableSample)
+ sample.child
+ } else {
+ throw new AnalysisException(
+ errorClass = "_LEGACY_ERROR_TEMP_0035",
Review Comment:
we should not throw Legacy in the new code
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownTableSample.java:
##########
@@ -36,4 +36,24 @@ boolean pushTableSample(
double upperBound,
boolean withReplacement,
long seed);
+
+ /**
+ * Pushes down SAMPLE to the data source with sample method awareness.
+ * Data sources can override this to distinguish SYSTEM (block) from BERNOULLI (row) sampling.
+ * By default, rejects SYSTEM sampling for backward compatibility and delegates BERNOULLI to
+ * the 4-parameter version.
+ */
+ default boolean pushTableSample(
+ double lowerBound,
+ double upperBound,
+ boolean withReplacement,
+ long seed,
+ boolean isSystemSampling) {
+ if (isSystemSampling) {
+ // If the data source hasn't overridden this method, it must have not added support
Review Comment:
not sure we need this general comment
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -811,7 +812,44 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
}
+ private def findScanBuilderHolder(plan: LogicalPlan): Option[ScanBuilderHolder] = {
+ plan match {
+ case s: ScanBuilderHolder => Some(s)
+ case s: SubqueryAlias => findScanBuilderHolder(s.child)
+ case p: Project => findScanBuilderHolder(p.child)
+ case f: Filter => findScanBuilderHolder(f.child)
Review Comment:
it seems different than PhysicalOperation and not correct to push through all Filters?
What about this scenario?
Sample(10%)
SubqueryAlias
Filter(x > 5)
ScanBuilderHolder(t)
Sample(10%) following Filter(x>5) is different than Filter(x>5) following Sample(10%)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3025177172
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -811,7 +812,44 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
}
+ private def findScanBuilderHolder(plan: LogicalPlan): Option[ScanBuilderHolder] = {
+ plan match {
+ case s: ScanBuilderHolder => Some(s)
+ case s: SubqueryAlias => findScanBuilderHolder(s.child)
+ case p: Project => findScanBuilderHolder(p.child)
+ case f: Filter => findScanBuilderHolder(f.child)
Review Comment:
Good point! Per the SQL standard (ISO/IEC 9075), TABLESAMPLE SYSTEM is applied to the table reference in the FROM clause, before WHERE filtering. When the FROM clause is a subquery/view with filters, it will scan the table, so block sampling won't be able to save I/O. Therefore block sampling is not meaningful in this case. Therefore, I fixed this by only matching ScanBuilderHolder or SubqueryAlias and won't push down SAMPLE through filter (or things like that). Otherwise, we will explicitly error out as unsupported feature.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3220885550
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -831,6 +837,30 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan] with PredicateHelper {
}
def pushDownSample(plan: LogicalPlan): LogicalPlan = plan.transform {
+ case sample: Sample if sample.sampleMethod == SampleMethod.System =>
+ sample.child match {
+ case PhysicalOperation(_, Nil, sHolder: ScanBuilderHolder) =>
+ val tableSample = TableSampleInfo(
+ sample.lowerBound,
+ sample.upperBound,
+ sample.withReplacement,
+ sample.seed.getOrElse((math.random() * 1000).toLong),
Review Comment:
I think it's fine to be consistent here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4257314472
> @stanyao took a look, it looks coming together.
>
> Do we have any plan to add test coverage using DSV2 InMemory data source? I think we should add it.
I added the following plan level test cases and harness:
* InMemoryTableWithTableSample.scala — in-memory DSv2 table with scan builder implementing SupportsPushDownTableSample (BERNOULLI + SYSTEM).
* InMemoryTableWithTableSampleCatalog.scala — catalog creating those tables.
* DataSourceV2TableSampleSuite.scala — 7 end-to-end tests: SYSTEM/BERNOULLI/default pushdown, 0%/100% boundary, projection composition, and non-pushdown fallback.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3048492073
##########
sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectDatabaseMetaDataSuite.scala:
##########
@@ -209,7 +209,7 @@ class SparkConnectDatabaseMetaDataSuite extends ConnectFunSuite with RemoteSpark
withConnection { conn =>
val metadata = conn.getMetaData
// scalastyle:off line.size.limit
- assert(metadata.getSQLKeywords === "ADD,AFTER,AGGREGATE,ALWAYS,ANALYZE,ANTI,ANY_VALUE,ARCHIVE,ASC,BINDING,BUCKET,BUCKETS,BYTE,CACHE,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,MET
RICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,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,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,M
EASURE,METRICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,WEEK,WEEKS,WHILE,X,YEARS,ZONE")
Review Comment:
@szehon-ho , Spark Connect is outside the scope of this PR originally. Is it OK I log a JIRA item to track a follow up addition of Spark Connect support? I also plan to log another JIRA item to add DataFrame API support too.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3121073632
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2TableSampleSuite.scala:
##########
@@ -0,0 +1,98 @@
+/*
+ * 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.
+ */
+
+package org.apache.spark.sql.connector
+
+import org.apache.spark.sql.connector.catalog.InMemoryTableWithTableSampleCatalog
+
+class DataSourceV2TableSampleSuite extends DatasourceV2SQLBase
Review Comment:
I assume by "pushed systemtable EXPLAIN", you meant validating when a system sample is pushed down, the explain plan will include expected sample information like "SYSTEM" and "BERNOULLI" and their parameters. To simplify, I added that coverage in the following existing test cases:
- test("SPARK-55978: TABLESAMPLE SYSTEM pushdown removes Sample node")
- test("SPARK-55978: TABLESAMPLE BERNOULLI pushdown removes Sample node")
Added more test cases below for the other error case related comment above:
- non-pushdown DSv2 catalog => UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM
- subquery => UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_NO_SCAN
- DSv1 parquet table => UNSUPPORTED_FEATURE.TABLESAMPLE_SYSTEM_NO_SCAN
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on PR #54972:
URL: https://github.com/apache/spark/pull/54972#issuecomment-4297863001
Added additional test coverage:
- "SYSTEM + BUCKET ON Col" should produce expected error
- "SYSTEM + WHERE filter" should produce expected error
- An additional test case for Join pushdown guard during "join + sample"
- Legacy connector (DSv2 connector created before this PR) backward compat: Create a test catalog whose ScanBuilder implements only the 4-arg pushTableSample (does NOT override the 5-arg default). Verify BERNOULLI still pushes down via the default delegation, and SYSTEM errors with TABLESAMPLE_SYSTEM.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55978][SQL] Add TABLESAMPLE SYSTEM block sampling with DSv2 pushdown [spark]
Posted by "stanyao (via GitHub)" <gi...@apache.org>.
stanyao commented on code in PR #54972:
URL: https://github.com/apache/spark/pull/54972#discussion_r3048492073
##########
sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectDatabaseMetaDataSuite.scala:
##########
@@ -209,7 +209,7 @@ class SparkConnectDatabaseMetaDataSuite extends ConnectFunSuite with RemoteSpark
withConnection { conn =>
val metadata = conn.getMetaData
// scalastyle:off line.size.limit
- assert(metadata.getSQLKeywords === "ADD,AFTER,AGGREGATE,ALWAYS,ANALYZE,ANTI,ANY_VALUE,ARCHIVE,ASC,BINDING,BUCKET,BUCKETS,BYTE,CACHE,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,MET
RICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,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,CACHED,CASCADE,CATALOG,CATALOGS,CHANGE,CHANGES,CLEAR,CLUSTER,CLUSTERED,CODEGEN,COLLATION,COLLATIONS,COLLECTION,COLUMNS,COMMENT,COMPACT,COMPACTIONS,COMPENSATION,COMPUTE,CONCATENATE,CONTAINS,CONTINUE,COST,DATA,DATABASE,DATABASES,DATEADD,DATEDIFF,DATE_ADD,DATE_DIFF,DAYOFYEAR,DAYS,DBPROPERTIES,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,M
EASURE,METRICS,MICROSECOND,MICROSECONDS,MILLISECOND,MILLISECONDS,MINUS,MINUTES,MONTHS,MSCK,NAME,NAMESPACE,NAMESPACES,NANOSECOND,NANOSECONDS,NORELY,NULLS,OFFSET,OPTION,OPTIONS,OUTPUTFORMAT,OVERWRITE,PARTITIONED,PARTITIONS,PERCENT,PIVOT,PLACING,PRECEDING,PRINCIPALS,PROCEDURES,PROPERTIES,PURGE,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_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,WATERMARK,WEEK,WEEKS,WHILE,X,YEARS,ZONE")
Review Comment:
@szehon-ho , Spark Connect is outside the scope of this PR. It will require changes in wire protocol and all language clients, which is worth a focused review. Is it OK I log a JIRA item to track a follow up addition of Spark Connect support? I also plan to log another JIRA item to add DataFrame API support too.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org