Skip to content

[SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter - #37197

Closed
huaxingao wants to merge 8 commits into
apache:masterfrom
huaxingao:flip
Closed

Conversation

@huaxingao

@huaxingaohuaxingao commented Jul 15, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Even though the literal value could be on both sides of the filter, e.g. both a > 1 and 1 < a are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.

Before this PR, for the filters that have literal values on the right side, e.g. 1 > a, we keep it as is. After this PR, we will normalize it to a < 1 so the data source doesn't need to check each of the filters (and do the flip).

Why are the changes needed?

I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.

Does this PR introduce any user-facing change?

no

How was this patch tested?

new test

@huaxingao

huaxingao commented Jul 15, 2022

Copy link
Copy Markdown
ContributorAuthor

cc @cloud-fan

@cloud-fan

cloud-fan commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

Is this still a limitation in the v2 filter? Or this is just for normalization so that the sources can handle v2 filters easier?

@huaxingao

huaxingao commented Jul 15, 2022

Copy link
Copy Markdown
ContributorAuthor

It is just for normalization so that the sources can handle v2 filters easier.
V1 filter does this too. https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala#L519

dongjoon-hyun
dongjoon-hyun reviewed Jul 20, 2022
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate more into the PR description about what happened before this PR, @huaxingao ?

dongjoon-hyun
dongjoon-hyun reviewed Jul 20, 2022
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, in this case, we should not use should in the PR title, @huaxingao .

It is just for normalization so that the sources can handle v2 filters easier.

@dongjoon-hyun

dongjoon-hyun commented Jul 20, 2022

Copy link
Copy Markdown
Member

cc @sunchao too

@huaxingaohuaxingao changed the title [SPARK-39784][SQL] Literal values should be on the right side of the data source filter[SPARK-39784][SQL] Literal values should be on the right side of the data source filter after translating Catalyst Expression to data source filterJul 20, 2022
@huaxingaohuaxingao changed the title [SPARK-39784][SQL] Literal values should be on the right side of the data source filter after translating Catalyst Expression to data source filter[SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filterJul 20, 2022
@huaxingao

huaxingao commented Jul 20, 2022

Copy link
Copy Markdown
ContributorAuthor

Updated. Thanks for taking a look @dongjoon-hyun

dongjoon-hyun
dongjoon-hyun reviewed Jul 20, 2022
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating. I'll test more tonight, @huaxingao .

zinking
zinking reviewed Jul 21, 2022
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala Outdated
case "<" => ">"
case ">=" => "<="
case "<=" => ">="
case _ => filterName

@zinkingzinkingJul 21, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is = handled as well ?

@dongjoon-hyundongjoon-hyunJul 21, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to flip the operator, =, do we? @zinking
The operands are flipped already via Array(expr.children()(1), expr.children()(0)).

zinking
zinking reviewed Jul 21, 2022
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala Outdated
case PushablePredicate(expr) => Some(expr)
case PushablePredicate(expr) =>
if (expr.children().length == 2) {
expr.children()(0) match {

@zinkingzinkingJul 21, 2022•
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be written into something like expr.children() match { case Literal :: xs =>

sunchao
sunchao reviewed Jul 21, 2022
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala Outdated
if (expr.children().length == 2) {
expr.children()(0) match {
case LiteralValue(_, _) =>
Some(new Predicate(flipComparisonFilterName(expr.name()),

@sunchaosunchaoJul 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should be more strict and only flip the filter iff the operator is in the set { >, <, >=, <=}

@cloud-fan

cloud-fan commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

shall we do this normalization in V2ExpressionBuilder?

dongjoon-hyun
dongjoon-hyun reviewed Jul 21, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 21, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 21, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 22, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 22, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated
dongjoon-hyun
dongjoon-hyun reviewed Jul 22, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala Outdated
@huaxingao

huaxingao commented Jul 22, 2022

Copy link
Copy Markdown
ContributorAuthor

The R dependency failure is unrelated to this PR.

dongjoon-hyun
dongjoon-hyun approved these changes Jul 22, 2022
View reviewed changes

@dongjoon-hyundongjoon-hyun left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @huaxingao . The UT failure is irrelevant and fixed in the master branch already.

sunchao
sunchao approved these changes Jul 22, 2022
View reviewed changes

@sunchaosunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too

@huaxingao

huaxingao commented Jul 22, 2022

Copy link
Copy Markdown
ContributorAuthor

Merged to master. Thank you all very much!

cloud-fan
cloud-fan reviewed Jul 25, 2022
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/catalyst/util/V2ExpressionBuilder.scala
val r = generateExpression(b.right)
if (l.isDefined && r.isDefined) {
b match {
case _: Predicate if isBinaryComparisonOperator(b.sqlOperator) &&

@cloud-fancloud-fanJul 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can just use type check BinaryComparison

@huaxingaohuaxingaoJul 25, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will submit a follow up to fix this. Thanks.

@huaxingaohuaxingao mentioned this pull request Jul 25, 2022
Closed
dongjoon-hyun pushed a commit that referenced this pull request Jul 25, 2022
@huaxingao@dongjoon-hyun
[SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predica…
8628c15
…te (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closes#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
a0x8o added a commit to a0x8o/spark that referenced this pull request Jul 25, 2022
@a0x8o
[SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predica…
cc8e7b2
…te (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache/spark#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closes #37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
chenzhx pushed a commit to chenzhx/spark that referenced this pull request Aug 15, 2022
@huaxingao@chenzhx
[SPARK-39784][SQL] Put Literal values on the right side of the data s…
5c7aee0
…ource filter after translating Catalyst Expression to data source filter
### What changes were proposed in this pull request?
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
### Why are the changes needed?
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
chenzhx pushed a commit to chenzhx/spark that referenced this pull request Aug 15, 2022
@huaxingao@chenzhx
[SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predica…
0aa6e44
…te (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
chenzhx added a commit to Kyligence/spark that referenced this pull request Aug 17, 2022
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashi
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
fcdda00
…aging (Sort with expressions) (#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
### What changes were proposed in this pull request?
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
### Why are the changes needed?
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
### What changes were proposed in this pull request?
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
### Why are the changes needed?
Simplify `V2ExpressionBuilder` by extract common method.
### Does this PR introduce _any_ user-facing change?
'No'.
Just update inner implementation.
### How was this patch tested?
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
### What changes were proposed in this pull request?
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
### Why are the changes needed?
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
### Does this PR introduce _any_ user-facing change?
'No'.
Just improve the inner implementation.
### How was this patch tested?
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
### What changes were proposed in this pull request?
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
### Why are the changes needed?
It can help us check the results individually and make the code more clearer.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
### What changes were proposed in this pull request?
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
### Why are the changes needed?
Add the range for DS V2 push down `Cast`.
### Does this PR introduce _any_ user-facing change?
'Yes'.
`Cast` could be pushed down to data source in more cases.
### How was this patch tested?
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
### What changes were proposed in this pull request?
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
### Why are the changes needed?
DS V2 supports push down misc functions.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
### What changes were proposed in this pull request?
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
### Why are the changes needed?
Unify the translate path for DS V2 pushdown.
### Does this PR introduce _any_ user-facing change?
'No'.
Just update the inner implementation.
### How was this patch tested?
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
### What changes were proposed in this pull request?
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
### Why are the changes needed?
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
### What changes were proposed in this pull request?
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
### Why are the changes needed?
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
chenzhx added a commit to chenzhx/spark that referenced this pull request Sep 15, 2022
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashiZhixiong Chen
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
7001605
…aging (Sort with expressions) (Kyligence#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
### What changes were proposed in this pull request?
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
### Why are the changes needed?
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
### What changes were proposed in this pull request?
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
### Why are the changes needed?
Simplify `V2ExpressionBuilder` by extract common method.
### Does this PR introduce _any_ user-facing change?
'No'.
Just update inner implementation.
### How was this patch tested?
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
### What changes were proposed in this pull request?
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
### Why are the changes needed?
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
### Does this PR introduce _any_ user-facing change?
'No'.
Just improve the inner implementation.
### How was this patch tested?
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
### What changes were proposed in this pull request?
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
### Why are the changes needed?
It can help us check the results individually and make the code more clearer.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
### What changes were proposed in this pull request?
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
### Why are the changes needed?
Add the range for DS V2 push down `Cast`.
### Does this PR introduce _any_ user-facing change?
'Yes'.
`Cast` could be pushed down to data source in more cases.
### How was this patch tested?
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
### What changes were proposed in this pull request?
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
### Why are the changes needed?
DS V2 supports push down misc functions.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
### What changes were proposed in this pull request?
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
### Why are the changes needed?
Unify the translate path for DS V2 pushdown.
### Does this PR introduce _any_ user-facing change?
'No'.
Just update the inner implementation.
### How was this patch tested?
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
### What changes were proposed in this pull request?
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
### Why are the changes needed?
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
### What changes were proposed in this pull request?
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
### Why are the changes needed?
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
chenzhx added a commit to Kyligence/spark that referenced this pull request Sep 15, 2022
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashi
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
eceeb6f
…aging (Sort with expressions) (#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
### What changes were proposed in this pull request?
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
### Why are the changes needed?
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
### What changes were proposed in this pull request?
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
### Why are the changes needed?
Simplify `V2ExpressionBuilder` by extract common method.
### Does this PR introduce _any_ user-facing change?
'No'.
Just update inner implementation.
### How was this patch tested?
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
### What changes were proposed in this pull request?
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
### Why are the changes needed?
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
### Does this PR introduce _any_ user-facing change?
'No'.
Just improve the inner implementation.
### How was this patch tested?
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
### What changes were proposed in this pull request?
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
### Why are the changes needed?
It can help us check the results individually and make the code more clearer.
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
### What changes were proposed in this pull request?
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
### Why are the changes needed?
Add the range for DS V2 push down `Cast`.
### Does this PR introduce _any_ user-facing change?
'Yes'.
`Cast` could be pushed down to data source in more cases.
### How was this patch tested?
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
### What changes were proposed in this pull request?
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
### Why are the changes needed?
DS V2 supports push down misc functions.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
### What changes were proposed in this pull request?
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
### Why are the changes needed?
Unify the translate path for DS V2 pushdown.
### Does this PR introduce _any_ user-facing change?
'No'.
Just update the inner implementation.
### How was this patch tested?
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
### What changes were proposed in this pull request?
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
### Why are the changes needed?
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
### What changes were proposed in this pull request?
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
### Why are the changes needed?
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
leejaywei pushed a commit to Kyligence/spark that referenced this pull request Oct 18, 2022
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashi@leejaywei
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
04f5c40
…aging (Sort with expressions) (#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
no
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
Simplify `V2ExpressionBuilder` by extract common method.
'No'.
Just update inner implementation.
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
'No'.
Just improve the inner implementation.
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
follow up this [comment](apache#37197 (comment))
code simplification
No
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
It can help us check the results individually and make the code more clearer.
no
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
Add the range for DS V2 push down `Cast`.
'Yes'.
`Cast` could be pushed down to data source in more cases.
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
DS V2 supports push down misc functions.
'No'.
New feature.
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
Unify the translate path for DS V2 pushdown.
'No'.
Just update the inner implementation.
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
'No'.
New feature.
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
'No'.
New feature.
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
a0x8o added a commit to a0x8o/spark that referenced this pull request Dec 30, 2022
@a0x8o
[SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predica…
cbe0231
…te (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache/spark#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closes #37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
a0x8o added a commit to a0x8o/spark that referenced this pull request Dec 30, 2022
@a0x8o
[SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predica…
7801420
…te (if) for type check
### What changes were proposed in this pull request?
follow up this [comment](apache/spark#37197 (comment))
### Why are the changes needed?
code simplification
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test
Closes #37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
hellozepp pushed a commit to hellozepp/spark that referenced this pull request Aug 10, 2023
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashi@hellozepp
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
fa08547
…aging (Sort with expressions) (Kyligence#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
no
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
Simplify `V2ExpressionBuilder` by extract common method.
'No'.
Just update inner implementation.
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
'No'.
Just improve the inner implementation.
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
follow up this [comment](apache#37197 (comment))
code simplification
No
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
It can help us check the results individually and make the code more clearer.
no
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
Add the range for DS V2 push down `Cast`.
'Yes'.
`Cast` could be pushed down to data source in more cases.
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
DS V2 supports push down misc functions.
'No'.
New feature.
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
Unify the translate path for DS V2 pushdown.
'No'.
Just update the inner implementation.
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
'No'.
New feature.
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
'No'.
New feature.
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
zheniantoushipashi added a commit to Kyligence/spark that referenced this pull request Aug 21, 2023
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashi
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
4a48998
…aging (Sort with expressions) (#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
no
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
Simplify `V2ExpressionBuilder` by extract common method.
'No'.
Just update inner implementation.
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
'No'.
Just improve the inner implementation.
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
follow up this [comment](apache#37197 (comment))
code simplification
No
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
It can help us check the results individually and make the code more clearer.
no
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
Add the range for DS V2 push down `Cast`.
'Yes'.
`Cast` could be pushed down to data source in more cases.
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
DS V2 supports push down misc functions.
'No'.
New feature.
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
Unify the translate path for DS V2 pushdown.
'No'.
Just update the inner implementation.
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
'No'.
New feature.
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
'No'.
New feature.
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
RolatZhang pushed a commit to Kyligence/spark that referenced this pull request Aug 29, 2023
@chenzhx@huaxingao
@beliefer@yabola@zheniantoushipashi@RolatZhang
[SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or P…
f981503
…aging (Sort with expressions) (#525)
* [SPARK-39784][SQL] Put Literal values on the right side of the data source filter after translating Catalyst Expression to data source filter
Even though the literal value could be on both sides of the filter, e.g. both `a > 1` and `1 < a` are valid, after translating Catalyst Expression to data source filter, we want the literal value on the right side so it's easier for the data source to handle these filters. We do this kind of normalization for V1 Filter. We should have the same behavior for V2 Filter.
Before this PR, for the filters that have literal values on the right side, e.g. `1 > a`, we keep it as is. After this PR, we will normalize it to `a < 1` so the data source doesn't need to check each of the filters (and do the flip).
I think we should follow V1 Filter's behavior, normalize the filters during catalyst Expression to DS Filter translation time to make the literal values on the right side, so later on, data source doesn't need to check every single filter to figure out if it needs to flip the sides.
no
new test
Closesapache#37197 from huaxingao/flip.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39836][SQL] Simplify V2ExpressionBuilder by extract common method
Currently, `V2ExpressionBuilder` have a lot of similar code, we can extract them as one common method.
We can simplify the implement with the common method.
Simplify `V2ExpressionBuilder` by extract common method.
'No'.
Just update inner implementation.
N/A
Closesapache#37249 from beliefer/SPARK-39836.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39858][SQL] Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
When I using `AliasHelper`, I find that some rules inherit it instead of using it.
This PR removes unnecessary `AliasHelper` or `PredicateHelper` in the following cases:
- The rule inherit `AliasHelper` instead of using it. In this case, we can remove `AliasHelper` directly.
- The rule inherit `PredicateHelper` instead of using it. In this case, we can remove `PredicateHelper` directly.
- The rule inherit `AliasHelper` and `PredicateHelper`. In fact, `PredicateHelper` already extends `AliasHelper`. In this case, we can remove `AliasHelper`.
- The rule inherit `OperationHelper` and `PredicateHelper`. In fact, `OperationHelper` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `PlanTest` and `PredicateHelper`. In fact, `PlanTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
- The rule inherit `QueryTest` and `PredicateHelper`. In fact, `QueryTest` already extends `PredicateHelper`. In this case, we can remove `PredicateHelper`.
Remove unnecessary `AliasHelper` or `PredicateHelper` for some rules
'No'.
Just improve the inner implementation.
N/A
Closesapache#37272 from beliefer/SPARK-39858.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39784][SQL][FOLLOW-UP] Use BinaryComparison instead of Predicate (if) for type check
follow up this [comment](apache#37197 (comment))
code simplification
No
Existing test
Closesapache#37278 from huaxingao/followup.
Authored-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-39909] Organize the check of push down information for JDBCV2Suite
This PR changes the check method from `check(one_large_string)` to `check(small_string1, small_string2, ...)`
It can help us check the results individually and make the code more clearer.
no
existing tests
Closesapache#37342 from yabola/fix.
Authored-by: chenliang.lu <marssss2929@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
* [SPARK-39961][SQL] DS V2 push-down translate Cast if the cast is safe
Currently, DS V2 push-down translate `Cast` only if the ansi mode is true.
In fact, if the cast is safe(e.g. cast number to string, cast int to long), we can translate it too.
This PR will call `Cast.canUpCast` so as we can translate `Cast` to V2 `Cast` safely.
Note: The rule `SimplifyCasts` optimize some safe cast, e.g. cast int to long, so we may not see the `Cast`.
Add the range for DS V2 push down `Cast`.
'Yes'.
`Cast` could be pushed down to data source in more cases.
Test cases updated.
Closesapache#37388 from beliefer/SPARK-39961.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
* [SPARK-38901][SQL] DS V2 supports push down misc functions
Currently, Spark have some misc functions. Please refer
https://github.com/apache/spark/blob/2f8613f22c0750c00cf1dcfb2f31c431d8dc1be7/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala#L688
These functions show below:
`AES_ENCRYPT,`
`AES_DECRYPT`,
`SHA1`,
`SHA2`,
`MD5`,
`CRC32`
Function|PostgreSQL|ClickHouse|H2|MySQL|Oracle|Redshift|Snowflake|DB2|Vertica|Exasol|SqlServer|Yellowbrick|Mariadb|Singlestore|
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
`AesEncrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`AesDecrypt`|Yes|Yes|Yes|Yes|Yes|NO|Yes|Yes|NO|NO|NO|Yes|Yes|Yes|
`Sha1`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Sha2`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Md5`|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|Yes|
`Crc32`|No|Yes|No|Yes|NO|Yes|NO|Yes|NO|NO|NO|NO|NO|Yes|
DS V2 should supports push down these misc functions.
DS V2 supports push down misc functions.
'No'.
New feature.
New tests.
Closesapache#37169 from chenzhx/misc.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39964][SQL] DS V2 pushdown should unify the translate path
Currently, DS V2 pushdown have two translate path `DataSourceStrategy.translateAggregate` used to translate aggregate functions and `V2ExpressionBuilder` used to translate other functions and expressions, we can unify them.
After this PR, the translate have only one code path, developers will easy to coding and reading.
Unify the translate path for DS V2 pushdown.
'No'.
Just update the inner implementation.
N/A
Closesapache#37391 from beliefer/SPARK-39964.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39819][SQL] DS V2 aggregate push down can work with Top N or Paging (Sort with expressions)
Currently, DS V2 aggregate push-down cannot work with DS V2 Top N push-down (`ORDER BY col LIMIT m`) or DS V2 Paging push-down (`ORDER BY col LIMIT m OFFSET n`).
If we can push down aggregate with Top N or Paging, it will be better performance.
This PR only let aggregate pushed down with ORDER BY expressions which must be GROUP BY expressions.
The idea of this PR are:
1. When we give an expectation outputs of `ScanBuilderHolder`, holding the map from expectation outputs to origin expressions (contains origin columns).
2. When we try to push down Top N or Paging, we need restore the origin expressions for `SortOrder`.
Let DS V2 aggregate push down can work with Top N or Paging (Sort with group expressions), then users can get the better performance.
'No'.
New feature.
New test cases.
Closesapache#37320 from beliefer/SPARK-39819_new.
Authored-by: Jiaan Geng <beliefer@163.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-39929][SQL] DS V2 supports push down string functions(non ANSI)
**What changes were proposed in this pull request?**
support more commonly used string functions
BIT_LENGTH
CHAR_LENGTH
CONCAT
The mainstream databases support these functions show below.
Function | PostgreSQL | ClickHouse | H2 | MySQL | Oracle | Redshift | Presto | Teradata | Snowflake | DB2 | Vertica | Exasol | SqlServer | Yellowbrick | Impala | Mariadb | Druid | Pig | SQLite | Influxdata | Singlestore | ElasticSearch
-- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | -- | --
BIT_LENGTH | Yes | Yes | Yes | Yes | Yes | no | no | no | no | Yes | Yes | Yes | no | Yes | no | Yes | no | no | no | no | no | Yes
CHAR_LENGTH | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | Yes | Yes | Yes | Yes
CONCAT | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | Yes | no | no | no | Yes | Yes
**Why are the changes needed?**
DS V2 supports push down string functions
**Does this PR introduce any user-facing change?**
'No'.
New feature.
How was this patch tested?
New tests.
Closesapache#37427 from zheniantoushipashi/SPARK-39929.
Authored-by: biaobiao.sun <1319027852@qq.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* [SPARK-38899][SQL][FOLLOWUP] Fix bug extract datetime in DS V2 pushdown
[SPARK-38899](apache#36663) supports extract function in JDBC data source.
But the implement is incorrect.
This PR just add a test case and it will be failed !
The test case show below.
```
test("scan with filter push-down with date time functions") {
val df9 = sql("SELECT name FROM h2.test.datetime WHERE " +
"dayofyear(date1) > 100 order by dayofyear(date1) limit 1")
checkFiltersRemoved(df9)
val expectedPlanFragment9 =
"PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], " +
"PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
checkPushedInfo(df9, expectedPlanFragment9)
checkAnswer(df9, Seq(Row("alex")))
}
```
The test case output failure show below.
```
"== Parsed Logical Plan ==
'GlobalLimit 1
+- 'LocalLimit 1
+- 'Sort ['dayofyear('date1) ASC NULLS FIRST], true
+- 'Project ['name]
+- 'Filter ('dayofyear('date1) > 100)
+- 'UnresolvedRelation [h2, test, datetime], [], false
== Analyzed Logical Plan ==
name: string
GlobalLimit 1
+- LocalLimit 1
+- Project [name#x]
+- Sort [dayofyear(date1#x) ASC NULLS FIRST], true
+- Project [name#x, date1#x]
+- Filter (dayofyear(date1#x) > 100)
+- SubqueryAlias h2.test.datetime
+- RelationV2[NAME#x, DATE1#x, TIME1#x] h2.test.datetime test.datetime
== Optimized Logical Plan ==
Project [name#x]
+- RelationV2[NAME#x] test.datetime
== Physical Plan ==
*(1) Scan org.apache.spark.sql.execution.datasources.v2.jdbc.JDBCScan$$anon$145f6181a [NAME#x] PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [org.apache.spark.sql.connector.expressions.Extract3b95fce9 ASC NULLS FIRST] LIMIT 1, ReadSchema: struct<NAME:string>
" did not contain "PushedFilters: [DATE1 IS NOT NULL, EXTRACT(DAY_OF_YEAR FROM DATE1) > 100], PushedTopN: ORDER BY [EXTRACT(DAY_OF_YEAR FROM DATE1) ASC NULLS FIRST] LIMIT 1,"
```
Fix an implement bug.
The reason of the bug is the Extract the function does not implement the toString method when pushing down the JDBC data source.
'No'.
New feature.
New test case.
Closesapache#37469 from chenzhx/spark-master.
Authored-by: chenzhx <chen@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
* code update
Signed-off-by: huaxingao <huaxin_gao@apple.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Co-authored-by: huaxingao <huaxin_gao@apple.com>
Co-authored-by: Jiaan Geng <beliefer@163.com>
Co-authored-by: chenliang.lu <marssss2929@gmail.com>
Co-authored-by: biaobiao.sun <1319027852@qq.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan left review comments
@sunchaosunchaosunchao approved these changes
@dongjoon-hyundongjoon-hyundongjoon-hyun approved these changes
+1 more reviewer
@zinkingzinkingzinking left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@huaxingao@cloud-fan@dongjoon-hyun@zinking@sunchao