You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/04/24 06:00:08 UTC
[GitHub] [spark] huaxingao opened a new pull request, #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
huaxingao opened a new pull request, #36332:
URL: https://github.com/apache/spark/pull/36332
Co-Authored-By: DB Tsai [d_tsai@apple.com](mailto:d_tsai@apple.com)
Co-Authored-By: Huaxin Gao [huaxin_gao@apple.com](mailto:huaxin_gao@apple.com)
### What changes were proposed in this pull request?
updated the V2 file source to use V2 Filters. ParquetFilter hasn't been updated to use the V2 Filters yet and will be changed in the next PR.
### Why are the changes needed?
V2 Filter migration
### Does this PR introduce _any_ user-facing change?
no
### How was this patch tested?
New and existing test suites
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857304020
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
+ String expressionStr = "";
+ for (Expression e : children()) {
+ expressionStr += e.describe() + ", ";
+ }
+
+ if (name().equals("=")) {
+ if (children()[1] instanceof LiteralValue) {
+ // e.g. a = 1
+ return new EqualTo(children()[0].describe(),
+ CatalystTypeConverters.convertToScala(((LiteralValue)children()[1]).value(),
Review Comment:
Two indents
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/csv/CSVScan.scala:
##########
@@ -70,8 +70,8 @@ case class CSVScan(
ExprUtils.verifyColumnNameOfCorruptRecord(dataSchema, parsedOptions.columnNameOfCorruptRecord)
// Don't push any filter which refers to the "virtual" column which cannot present in the input.
// Such filters will be applied later on the upper layer.
- val actualFilters =
- pushedFilters.filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
+ val actualFilters = pushedFilters.map(_.toV1)
+ .filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
Review Comment:
+1
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
+ String expressionStr = "";
+ for (Expression e : children()) {
+ expressionStr += e.describe() + ", ";
+ }
+
+ if (name().equals("=")) {
+ if (children()[1] instanceof LiteralValue) {
+ // e.g. a = 1
+ return new EqualTo(children()[0].describe(),
+ CatalystTypeConverters.convertToScala(((LiteralValue)children()[1]).value(),
+ ((LiteralValue)children()[1]).dataType()));
+ } else if (children()[0] instanceof LiteralValue) {
+ // e.g. 1 = a
+ return new EqualTo(children()[1].describe(),
+ CatalystTypeConverters.convertToScala(((LiteralValue)children()[0]).value(),
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #36332:
URL: https://github.com/apache/spark/pull/36332#issuecomment-1287960765
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
URL: https://github.com/apache/spark/pull/36332
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on PR #36332:
URL: https://github.com/apache/spark/pull/36332#issuecomment-1108754421
> Uh, I just want to know why we not refactor FileScanBuilder by replaces SupportsPushDownCatalystFilters to SupportsPushDownV2Filters ?
We actually intentionally want to push down catalyst Expression instead of filter in file source, because in file source we need to do partition pruning, which uses catalyst Expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857766838
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/csv/CSVScan.scala:
##########
@@ -70,8 +70,8 @@ case class CSVScan(
ExprUtils.verifyColumnNameOfCorruptRecord(dataSchema, parsedOptions.columnNameOfCorruptRecord)
// Don't push any filter which refers to the "virtual" column which cannot present in the input.
// Such filters will be applied later on the upper layer.
- val actualFilters =
- pushedFilters.filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
+ val actualFilters = pushedFilters.map(_.toV1)
+ .filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
Review Comment:
Added comments. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857303695
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/csv/CSVScan.scala:
##########
@@ -70,8 +70,8 @@ case class CSVScan(
ExprUtils.verifyColumnNameOfCorruptRecord(dataSchema, parsedOptions.columnNameOfCorruptRecord)
// Don't push any filter which refers to the "virtual" column which cannot present in the input.
// Such filters will be applied later on the upper layer.
- val actualFilters =
- pushedFilters.filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
+ val actualFilters = pushedFilters.map(_.toV1)
+ .filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
Review Comment:
Currently `OrcFilters`, `ParquetFilters`, `JacksonParser`, `UnivocityParser` only take v1 filters. There are actually quite some work to refactor these to make them also work with v2 filters. I prefer to have separate PRs later on for these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857552200
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
+ String expressionStr = "";
+ for (Expression e : children()) {
+ expressionStr += e.describe() + ", ";
Review Comment:
suggest using `StringBuilder` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857305367
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/csv/CSVScan.scala:
##########
@@ -70,8 +70,8 @@ case class CSVScan(
ExprUtils.verifyColumnNameOfCorruptRecord(dataSchema, parsedOptions.columnNameOfCorruptRecord)
// Don't push any filter which refers to the "virtual" column which cannot present in the input.
// Such filters will be applied later on the upper layer.
- val actualFilters =
- pushedFilters.filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
+ val actualFilters = pushedFilters.map(_.toV1)
+ .filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
Review Comment:
I see. Maybe we can create some JIRAs for these planed changes and put the JIRA number into some comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857766363
##########
connector/avro/src/main/scala/org/apache/spark/sql/v2/avro/AvroScan.scala:
##########
@@ -59,7 +59,7 @@ case class AvroScan(
readDataSchema,
readPartitionSchema,
parsedOptions,
- pushedFilters)
+ pushedFilters.map(_.toV1))
Review Comment:
right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857552200
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
+ String expressionStr = "";
+ for (Expression e : children()) {
+ expressionStr += e.describe() + ", ";
Review Comment:
should use `StringBuilder` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r859958160
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
Review Comment:
We shouldn't add this public API. Can we have a private internal util function to do it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] viirya commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
viirya commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857281246
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/csv/CSVScan.scala:
##########
@@ -70,8 +70,8 @@ case class CSVScan(
ExprUtils.verifyColumnNameOfCorruptRecord(dataSchema, parsedOptions.columnNameOfCorruptRecord)
// Don't push any filter which refers to the "virtual" column which cannot present in the input.
// Such filters will be applied later on the upper layer.
- val actualFilters =
- pushedFilters.filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
+ val actualFilters = pushedFilters.map(_.toV1)
+ .filterNot(_.references.contains(parsedOptions.columnNameOfCorruptRecord))
Review Comment:
Hmm, why we cannot work on v2 here and other places directly? I feel it is verbose and redundant to see `toV1`, `toV2` there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on PR #36332:
URL: https://github.com/apache/spark/pull/36332#issuecomment-1108104046
cc @cloud-fan @viirya @beliefer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
beliefer commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857310791
##########
connector/avro/src/main/scala/org/apache/spark/sql/v2/avro/AvroScan.scala:
##########
@@ -59,7 +59,7 @@ case class AvroScan(
readDataSchema,
readPartitionSchema,
parsedOptions,
- pushedFilters)
+ pushedFilters.map(_.toV1))
Review Comment:
It seems we no need `toV1` in future ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857773127
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
+ String expressionStr = "";
+ for (Expression e : children()) {
+ expressionStr += e.describe() + ", ";
+ }
+
+ if (name().equals("=")) {
+ if (children()[1] instanceof LiteralValue) {
+ // e.g. a = 1
+ return new EqualTo(children()[0].describe(),
+ CatalystTypeConverters.convertToScala(((LiteralValue)children()[1]).value(),
Review Comment:
I changed my ide java continuation indent to 4 for another project. Changed back to 2 space. All the indentation should be good now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] huaxingao commented on a diff in pull request #36332: [SPARK-36730][SQL] Use V2 Filter in V2 file source
Posted by GitBox <gi...@apache.org>.
huaxingao commented on code in PR #36332:
URL: https://github.com/apache/spark/pull/36332#discussion_r857773797
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/filter/Predicate.java:
##########
@@ -146,4 +149,210 @@ public class Predicate extends GeneralScalarExpression {
public Predicate(String name, Expression[] children) {
super(name, children);
}
+
+ public org.apache.spark.sql.sources.Filter toV1() {
+ String expressionStr = "";
+ for (Expression e : children()) {
+ expressionStr += e.describe() + ", ";
Review Comment:
Changed. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org