You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2025/10/12 18:21:34 UTC
[PR] [SPARK-XXXXX][SQL] Support DSv2 in PushVariantIntoScan [spark]
viirya opened a new pull request, #52578:
URL: https://github.com/apache/spark/pull/52578
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This patch goes to add DSv2 support to the optimization rule `PushVariantIntoScan`. The `PushVariantIntoScan` rule only supports DSv1 Parquet (`ParquetFileFormat`) source. It limits the effectiveness of variant type usage on DSv2.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Although #52522 tried to add DSv2 support recently, the implementation implicitly binds `pruneColumns` to this variant access pushdown which could cause unexpected errors on the DSv2 datasources which don't support that. We need an explicit API between Spark and DSv2 datasource for the feature.
#52522 also didn't test through this DSv2 variant pushdown feature actually on the built-in DSv2 Parquet datasource. This patch reverts #52522 and proposes a new approach with comprehensive test coverage.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
Added new unit test suites `PushVariantIntoScanV2Suite` and `PushVariantIntoScanV2VectorizedSuite`.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Generated-by: Claude Code v2.0.13
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3398575776
Thanks @viirya for the reply! If i understand correctly, the core concern is: “If Spark pushes a variant scan into a DSv2 source that doesn’t understand it, we’ll see unexpected errors.” I agree — that’s exactly the failure mode we should avoid.
Would it be acceptable if I add a planner-side guard so this only happens when the source explicitly opts in?
On tests: I understand my InMemoryTable test might have issues. Can we fix it to exercise the planner contract correctly, or do you consider a built-in DSv2 Parquet test required for DSv2 change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3395201088
> BTW, maybe the test case is generated by `Claude Code`?
>
> > Generated-by: Claude Code v2.0.13
Yes. This is co-authored with Claude Code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2426132432
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/VariantAccessInfo.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.types.StructType;
+
+/**
+ * Variant access information that describes how variant fields are accessed in a query.
+ * <p>
+ * This class captures the information needed by data sources to optimize reading variant columns.
+ * Instead of reading the entire variant value, the data source can read only the fields that
+ * are actually accessed, represented as a structured schema.
+ * <p>
+ * For example, if a query accesses `variant_get(v, '$.a', 'int')` and
+ * `variant_get(v, '$.b', 'string')`, the extracted schema would be
+ * `struct<0:int, 1:string>` where field ordinals correspond to the access order.
Review Comment:
```suggestion
* `struct<a:int, b:string>` where field ordinals correspond to the access order.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2426133925
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/VariantAccessInfo.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.types.StructType;
+
+/**
+ * Variant access information that describes how variant fields are accessed in a query.
+ * <p>
+ * This class captures the information needed by data sources to optimize reading variant columns.
+ * Instead of reading the entire variant value, the data source can read only the fields that
+ * are actually accessed, represented as a structured schema.
+ * <p>
+ * For example, if a query accesses `variant_get(v, '$.a', 'int')` and
+ * `variant_get(v, '$.b', 'string')`, the extracted schema would be
+ * `struct<0:int, 1:string>` where field ordinals correspond to the access order.
+ *
+ * @since 4.1.0
+ */
+@Evolving
+public final class VariantAccessInfo implements Serializable {
Review Comment:
this can be a java record?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3408651062
Thanks @huaxingao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3609614987
> > Not sure why getting the conclusion. Why it must be fully or not at all? I think it makes sense to partially push down.
>
> The API returns a boolean[], where each element indicates whether the corresponding VariantProjection can be pushed down.
>
> VariantProjection is defined as:
>
> ```
> public final class VariantProjection {
> private final String[] columnName; // like column path to the actual nested field in variant from root schema
> private final DataType expectedDataType;
> // ...
> }
> ```
Okay, it makes sense then. I thought you meant all variant accesses must be pushed down fully or not at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3427344674
Gentle ping, @aokolnychyi .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3408504777
Instead of reverting the previous commit, how about I just rebase the proposed approach on previous code. Is it okay to you @huaxingao?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3399980735
I would also like to clarify the situation a little bit. This PR was not proposed to fix after identifying those issues from the merged PR.
We discussed internally about to fill this DSv2 gap in the PushVariantIntoScan rule recently. I'm leading this effort. We decided to do that in last week and I started to work on it last Friday. The implementation was completed yesterday and I was going to submit this PR.
Before submitting this PR, I actually don't know there is independent work and it was merged. When I found that there was code conflicts, so I took a break and review that PR. After that, I found that PR has those issues and so continued to submit this PR.
If I know there is already others working on it, I won't work on it but will review. If so, maybe we can catch those issues and fix them in that PR. But for now, as I already have completed the work and submitted as a PR. I think it makes more sense to move forward with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3600778235
> There are a couple of issues with the new DSV2 rule SupportsPushDownVariants in this PR:
> 1. The mix-in trait should override ScanBuilder rather than Scan for consistency.
I am neutral on this. I can make a change to do this if all are agreed on this direction.
> 2. Variant type projections can only be fully pushed down or not pushed down at all. Since there are only two states, the API can be simplified to `boolean[] pushVariantProjections(VariantProjection[] projections);`.
Not sure why getting the conclusion. Why it must be fully or not at all? I think it makes sense to partially push down.
> Given these issues, we should improve the API before 4.1 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3395200458
cc @huaxingao , @cloud-fan too from the previous approach.
- https://github.com/apache/spark/pull/52522
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3398211655
Thanks @viirya for the PR! Appreciate you putting together an alternative. A couple of thoughts before we move forward:
1. Variant + column pruning are not special-cased semantics
- Variant is just another column in the projected schema from Spark’s perspective. When a source supports column pruning, Spark should prune all columns it doesn’t need, including Variant, exactly like it does for other types. I don't think we need to introduce a new interface to control if Variant should be pruned.
- If a particular DSv2 implementation does not support column pruning, then pushing Variant access down into that source is not meaningful; the primary benefit of Variant pushdown is to reduce the read surface via pruning.
2. On testing built-in Parquet vs. InMemoryTable
- The InMemoryTable tests were designed to validate the planner and API plumbing, to test that Spark forms the right pruned schema and pushdown contracts and the source receives them. It should be sufficient for testing.
- It's nice to add a smoke test with the built-in DSv2 Parquet to increase end-to-end confidence and catch regressions in common deployments. But I don't think it should be used as a judgement that a PR is not good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3613789171
May I ask what is our next step, @gengliangwang and @viirya , because @dbtsai casted a veto on RC2 due to this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2434378692
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -280,8 +281,13 @@ object PushVariantIntoScan extends Rule[LogicalPlan] {
relation @ LogicalRelationWithTable(
hadoopFsRelation@HadoopFsRelation(_, _, _, _, _: ParquetFileFormat, _), _)) =>
rewritePlan(p, projectList, filters, relation, hadoopFsRelation)
- case p@PhysicalOperation(projectList, filters, relation: DataSourceV2Relation) =>
- rewriteV2RelationPlan(p, projectList, filters, relation)
+
+ // BEGIN-V2-SUPPORT: DataSource V2 pattern matching with API
+ case p@PhysicalOperation(projectList, filters,
+ scanRelation @ DataSourceV2ScanRelation(
+ relation, scan: SupportsPushDownVariants, output, _, _)) =>
+ rewritePlanV2(p, projectList, filters, scanRelation, scan)
+ // END-V2-SUPPORT
Review Comment:
Ditto. This closing comment also looks a little too strict to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2426617482
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -26,9 +26,12 @@ import org.apache.spark.sql.catalyst.planning.PhysicalOperation
import org.apache.spark.sql.catalyst.plans.logical.{Filter, LogicalPlan, Project, Subquery}
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.catalyst.util.ResolveDefaultColumns
+import org.apache.spark.sql.connector.read.{SupportsPushDownVariants, VariantAccessInfo}
import org.apache.spark.sql.errors.QueryExecutionErrors
import org.apache.spark.sql.execution.datasources.parquet.ParquetFileFormat
-import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Relation
+// BEGIN-V2-SUPPORT: DataSource V2 imports
Review Comment:
Just added at the beginning to make the V2 code separately clear. I will remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3408769863
Rebased and updated the title to "Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants" to reflect it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3613832483
As a side note, in order to avoid another veto on RC3, we can revert this unstable and controversial improvement from `branch-4.1` (both #52578 and #52522) first if there is no way for all people to satisfy, @huaxingao , @cloud-fan , @viirya , @gengliangwang , @dbtsai . We can have enough time to stabilize in the `master` branch for Apache Spark 4.2.0 timeframe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #52578: [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants
URL: https://github.com/apache/spark/pull/52578
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3412812417
I'd like to take a look at this one too. I have some thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3395433161
Hmm, all tests are passing again.But what is this error on documentation generation pipeline?
```
[error] java.lang.IllegalArgumentException: Illegal group reference
[error] at java.base/java.util.regex.Matcher.appendExpandedReplacement(Matcher.java:1067)
[error] at java.base/java.util.regex.Matcher.appendReplacement(Matcher.java:907)
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2426618156
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/VariantAccessInfo.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.types.StructType;
+
+/**
+ * Variant access information that describes how variant fields are accessed in a query.
+ * <p>
+ * This class captures the information needed by data sources to optimize reading variant columns.
+ * Instead of reading the entire variant value, the data source can read only the fields that
+ * are actually accessed, represented as a structured schema.
+ * <p>
+ * For example, if a query accesses `variant_get(v, '$.a', 'int')` and
+ * `variant_get(v, '$.b', 'string')`, the extracted schema would be
+ * `struct<0:int, 1:string>` where field ordinals correspond to the access order.
+ *
+ * @since 4.1.0
+ */
+@Evolving
+public final class VariantAccessInfo implements Serializable {
Review Comment:
Looks okay. Is it necessary though?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3395645532
It seems due to java doc. I'm looking into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2434446724
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -280,8 +281,13 @@ object PushVariantIntoScan extends Rule[LogicalPlan] {
relation @ LogicalRelationWithTable(
hadoopFsRelation@HadoopFsRelation(_, _, _, _, _: ParquetFileFormat, _), _)) =>
rewritePlan(p, projectList, filters, relation, hadoopFsRelation)
- case p@PhysicalOperation(projectList, filters, relation: DataSourceV2Relation) =>
- rewriteV2RelationPlan(p, projectList, filters, relation)
+
+ // BEGIN-V2-SUPPORT: DataSource V2 pattern matching with API
Review Comment:
Let me remove these comments before merging. It is just for making the implementation clear locally at the beginning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3397886362
> > This patch reverts #52522 and proposes a new approach ...
>
> I feel reverting the original PR first, then submitting a brand-new PR makes the git history clearer
Okay for me. @dongjoon-hyun @cloud-fan @huaxingao What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3407410219
cc @aokolnychyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3398615399
> Thanks @viirya for the reply! If i understand correctly, the core concern is: “If Spark pushes a variant scan into a DSv2 source that doesn’t understand it, we’ll see unexpected errors.” I agree — that’s exactly the failure mode we should avoid.
>
> Would it be acceptable if I add a planner-side guard so this only happens when the source explicitly opts in?
Thanks @huaxingao for the discussion and understanding.
I think we need an explicit DSv2 API to make the contract between Spark and datasource implementation around this variant pushdown feature. That is this PR proposed to do.
> On tests: I understand my InMemoryTable test might have issues. Can we fix it to exercise the planner contract correctly, or do you consider a built-in DSv2 Parquet test required for DSv2 change?
That is also what this PR proposed to do, adding dedicated DSv2 Variant pushdown tests for both row-based and vectorized-based readers with good test coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2434386289
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -291,103 +297,146 @@ object PushVariantIntoScan extends Rule[LogicalPlan] {
filters: Seq[Expression],
relation: LogicalRelation,
hadoopFsRelation: HadoopFsRelation): LogicalPlan = {
+ val variants = new VariantInRelation
+
val schemaAttributes = relation.resolve(hadoopFsRelation.dataSchema,
hadoopFsRelation.sparkSession.sessionState.analyzer.resolver)
-
- // Collect variant fields from the relation output
- val variants = collectAndRewriteVariants(schemaAttributes)
+ val defaultValues = ResolveDefaultColumns.existenceDefaultValues(StructType(
+ schemaAttributes.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata))))
+ for ((a, defaultValue) <- schemaAttributes.zip(defaultValues)) {
+ variants.addVariantFields(a.exprId, a.dataType, defaultValue, Nil)
+ }
if (variants.mapping.isEmpty) return originalPlan
- // Collect requested fields from projections and filters
projectList.foreach(variants.collectRequestedFields)
filters.foreach(variants.collectRequestedFields)
// `collectRequestedFields` may have removed all variant columns.
if (variants.mapping.forall(_._2.isEmpty)) return originalPlan
- // Build attribute map with rewritten types
- val attributeMap = buildAttributeMap(schemaAttributes, variants)
-
- // Build new schema with variant types replaced by struct types
+ val attributeMap = schemaAttributes.map { a =>
+ if (variants.mapping.get(a.exprId).exists(_.nonEmpty)) {
+ val newType = variants.rewriteType(a.exprId, a.dataType, Nil)
+ val newAttr = AttributeReference(a.name, newType, a.nullable, a.metadata)(
+ qualifier = a.qualifier)
+ (a.exprId, newAttr)
+ } else {
+ // `relation.resolve` actually returns `Seq[AttributeReference]`, although the return type
+ // is `Seq[Attribute]`.
+ (a.exprId, a.asInstanceOf[AttributeReference])
+ }
+ }.toMap
val newFields = schemaAttributes.map { a =>
val dataType = attributeMap(a.exprId).dataType
StructField(a.name, dataType, a.nullable, a.metadata)
}
- // Update relation output attributes with new types
val newOutput = relation.output.map(a => attributeMap.getOrElse(a.exprId, a))
- // Update HadoopFsRelation's data schema so the file source reads the struct columns
val newHadoopFsRelation = hadoopFsRelation.copy(dataSchema = StructType(newFields))(
hadoopFsRelation.sparkSession)
val newRelation = relation.copy(relation = newHadoopFsRelation, output = newOutput.toIndexedSeq)
- // Build filter and project with rewritten expressions
buildFilterAndProject(newRelation, projectList, filters, variants, attributeMap)
}
- private def rewriteV2RelationPlan(
+ // BEGIN-V2-SUPPORT: DataSource V2 rewrite method using SupportsPushDownVariants API
+ // Key differences from V1 implementation:
+ // 1. V2 uses DataSourceV2ScanRelation instead of LogicalRelation
+ // 2. Uses SupportsPushDownVariants API instead of directly manipulating scan
+ // 3. Schema is already resolved in scanRelation.output (no need for relation.resolve())
+ // 4. Scan rebuilding is handled by the scan implementation via the API
+ private def rewritePlanV2(
originalPlan: LogicalPlan,
projectList: Seq[NamedExpression],
filters: Seq[Expression],
- relation: DataSourceV2Relation): LogicalPlan = {
+ scanRelation: DataSourceV2ScanRelation,
Review Comment:
Thank you for narrowing down to `DataSourceV2ScanRelation` from `DataSourceV2Relation`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2425114379
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/VariantAccessInfo.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.types.StructType;
+
+/**
+ * Variant access information that describes how variant fields are accessed in a query.
+ * <p>
+ * This class captures the information needed by data sources to optimize reading variant columns.
+ * Instead of reading the entire variant value, the data source can read only the fields that
+ * are actually accessed, represented as a structured schema.
+ * <p>
+ * For example, if a query accesses {@code variant_get(v, '$.a', 'int')} and
+ * {@code variant_get(v, '$.b', 'string')}, the extracted schema would be
+ * {@code struct<0:int, 1:string>} where field ordinals correspond to the access order.
Review Comment:
Ya, these three lines seem to be the root cause.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3439855154
Merged to master for Apache Spark 4.1.0-preview3.
We can revise this when the reviewers have more comments before releasing Spark 4.1.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3599106815
There are a couple of issues with the new DSV2 rule SupportsPushDownVariants in this PR:
1. The mix-in trait should override ScanBuilder rather than Scan for consistency.
2. Variant type projections can only be fully pushed down or not pushed down at all. Since there are only two states, the API can be simplified to `boolean[] pushVariantProjections(VariantProjection[] projections);`.
Given these issues, we should improve the API before 4.1 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2425212030
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/VariantAccessInfo.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.types.StructType;
+
+/**
+ * Variant access information that describes how variant fields are accessed in a query.
+ * <p>
+ * This class captures the information needed by data sources to optimize reading variant columns.
+ * Instead of reading the entire variant value, the data source can read only the fields that
+ * are actually accessed, represented as a structured schema.
+ * <p>
+ * For example, if a query accesses {@code variant_get(v, '$.a', 'int')} and
+ * {@code variant_get(v, '$.b', 'string')}, the extracted schema would be
+ * {@code struct<0:int, 1:string>} where field ordinals correspond to the access order.
Review Comment:
Yea, `{@code }` blocks are the issues. Not sure why this javadoc tag cannot be parsed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2426771322
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/VariantAccessInfo.java:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.connector.read;
+
+import java.io.Serializable;
+import java.util.Objects;
+
+import org.apache.spark.annotation.Evolving;
+import org.apache.spark.sql.types.StructType;
+
+/**
+ * Variant access information that describes how variant fields are accessed in a query.
+ * <p>
+ * This class captures the information needed by data sources to optimize reading variant columns.
+ * Instead of reading the entire variant value, the data source can read only the fields that
+ * are actually accessed, represented as a structured schema.
+ * <p>
+ * For example, if a query accesses `variant_get(v, '$.a', 'int')` and
+ * `variant_get(v, '$.b', 'string')`, the extracted schema would be
+ * `struct<0:int, 1:string>` where field ordinals correspond to the access order.
Review Comment:
It is actually the ordinals. It is how PushVariantIntoScan rewrites the type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3397542283
> This patch reverts https://github.com/apache/spark/pull/52522 and proposes a new approach ...
I feel reverting the original PR first, then submitting a brand-new PR makes the git history clearer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3609244120
> Not sure why getting the conclusion. Why it must be fully or not at all? I think it makes sense to partially push down.
The API returns a boolean[], where each element indicates whether the corresponding VariantProjection can be pushed down.
VariantProjection is defined as:
```
public final class VariantProjection {
private final String[] columnName; // like column path to the actual nested field in variant from root schema
private final DataType expectedDataType;
// ...
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3439858413
Thank you, @viirya and all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3398431922
> Thanks @viirya for the PR! Appreciate you putting together an alternative. A couple of thoughts before we move forward:
>
> 1. Variant + column pruning are not special-cased semantics
>
>
> * Variant is just another column in the projected schema from Spark’s perspective. When a source supports column pruning, Spark should prune all columns it doesn’t need, including Variant, exactly like it does for other types. I don't think we need to introduce a new interface to control if Variant should be pruned.
>
> * If a particular DSv2 implementation does not support column pruning, then pushing Variant access down into that source is not meaningful; the primary benefit of Variant pushdown is to reduce the read surface via pruning.
Column pruning is not the same as this variant scan pushdown. This feature basically changes the datatype (variant type to struct type) not just pruning fields from it. Mixing column pruning with variant scan pushdown causes confusion on API semantics and unexpected errors on DSv2 datasource implementation. Actually you can access all fields from a variant column but this variant scan pushdown still works effectively. It is not about reducing read surface but optimize variant access and usage in the read and the query plan.
No to mention that with current approach if it pushes a variant scan into a DSv2 implementation doesn't know about it, there will be unexpected errors.
> 2. On testing built-in Parquet vs. InMemoryTable
>
>
> * The InMemoryTable tests were designed to validate the planner and API plumbing, to test that Spark forms the right pruned schema and pushdown contracts and the source receives them. It should be sufficient for testing.
>
> * It's nice to add a smoke test with the built-in DSv2 Parquet to increase end-to-end confidence and catch regressions in common deployments. But I don't think it should be used as a judgement that a PR is not good.
No. The test just verifies the schema was changed. InMemoryTable/Scan doesn't actually do the variant pushdown. In other words, it just makes sure that you changed the variant type to struct type in the InMemoryTable. It doesn't actually test if the variant scan happens correctly or not, isn't? If the table/scan doesn't support it, what will happen? In other words, for example, we can simply change a variant type to any other type and keep the test passing because the schema is matched. Btw, there are actually issues on the test, but that is not the only issue going to revert the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3408620620
@viirya Sounds good to me. 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
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2434378312
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -280,8 +281,13 @@ object PushVariantIntoScan extends Rule[LogicalPlan] {
relation @ LogicalRelationWithTable(
hadoopFsRelation@HadoopFsRelation(_, _, _, _, _: ParquetFileFormat, _), _)) =>
rewritePlan(p, projectList, filters, relation, hadoopFsRelation)
- case p@PhysicalOperation(projectList, filters, relation: DataSourceV2Relation) =>
- rewriteV2RelationPlan(p, projectList, filters, relation)
+
+ // BEGIN-V2-SUPPORT: DataSource V2 pattern matching with API
Review Comment:
Do we need to introduce this comment prefix pattern, `BEGIN-V2-SUPPORT`, @viirya ? For me, this is new to Apache Spark community.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2434380187
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -291,103 +297,146 @@ object PushVariantIntoScan extends Rule[LogicalPlan] {
filters: Seq[Expression],
relation: LogicalRelation,
hadoopFsRelation: HadoopFsRelation): LogicalPlan = {
+ val variants = new VariantInRelation
+
val schemaAttributes = relation.resolve(hadoopFsRelation.dataSchema,
hadoopFsRelation.sparkSession.sessionState.analyzer.resolver)
-
- // Collect variant fields from the relation output
- val variants = collectAndRewriteVariants(schemaAttributes)
+ val defaultValues = ResolveDefaultColumns.existenceDefaultValues(StructType(
+ schemaAttributes.map(a => StructField(a.name, a.dataType, a.nullable, a.metadata))))
+ for ((a, defaultValue) <- schemaAttributes.zip(defaultValues)) {
+ variants.addVariantFields(a.exprId, a.dataType, defaultValue, Nil)
+ }
if (variants.mapping.isEmpty) return originalPlan
- // Collect requested fields from projections and filters
projectList.foreach(variants.collectRequestedFields)
filters.foreach(variants.collectRequestedFields)
// `collectRequestedFields` may have removed all variant columns.
if (variants.mapping.forall(_._2.isEmpty)) return originalPlan
- // Build attribute map with rewritten types
- val attributeMap = buildAttributeMap(schemaAttributes, variants)
-
- // Build new schema with variant types replaced by struct types
+ val attributeMap = schemaAttributes.map { a =>
+ if (variants.mapping.get(a.exprId).exists(_.nonEmpty)) {
+ val newType = variants.rewriteType(a.exprId, a.dataType, Nil)
+ val newAttr = AttributeReference(a.name, newType, a.nullable, a.metadata)(
+ qualifier = a.qualifier)
+ (a.exprId, newAttr)
+ } else {
+ // `relation.resolve` actually returns `Seq[AttributeReference]`, although the return type
+ // is `Seq[Attribute]`.
+ (a.exprId, a.asInstanceOf[AttributeReference])
+ }
+ }.toMap
val newFields = schemaAttributes.map { a =>
val dataType = attributeMap(a.exprId).dataType
StructField(a.name, dataType, a.nullable, a.metadata)
}
- // Update relation output attributes with new types
val newOutput = relation.output.map(a => attributeMap.getOrElse(a.exprId, a))
- // Update HadoopFsRelation's data schema so the file source reads the struct columns
val newHadoopFsRelation = hadoopFsRelation.copy(dataSchema = StructType(newFields))(
hadoopFsRelation.sparkSession)
val newRelation = relation.copy(relation = newHadoopFsRelation, output = newOutput.toIndexedSeq)
- // Build filter and project with rewritten expressions
buildFilterAndProject(newRelation, projectList, filters, variants, attributeMap)
}
- private def rewriteV2RelationPlan(
+ // BEGIN-V2-SUPPORT: DataSource V2 rewrite method using SupportsPushDownVariants API
Review Comment:
Ditto. This tag is simply surrounding this function.
```
// BEGIN-V2-SUPPORT:
// END-V2-SUPPORT
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3613920423
I think we just need to revise the current APIs a little bit (https://github.com/apache/spark/pull/53276) instead of reverting it as we would like to have this in 4.1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3600960206
>> 1. The mix-in trait should override ScanBuilder rather than Scan for consistency.
> I am neutral on this. I can make a change to do this if all are agreed on this direction.
For the refactor, I opened https://github.com/apache/spark/pull/53276.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3613800097
Also, cc @huaxingao and @cloud-fan from the original PR.
- #52522
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3614236850
Thank you, @viirya . It would be great if that's enough.
However, if you don't mind, could you get @dbtsai 's explicitly confirmation because he is the one who sent -1 on RC2? Also, @gengliangwang 's confirmation too.
It could be very weird if we get a repeated `-1` from the PMC members on RC3 due to this. It's because it means our consensus wasn't enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Fix DSv2 in PushVariantIntoScan by adding SupportsPushDownVariants [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3614242102
cc @aokolnychyi too because he also seems to have different opinion as the Apache Spark committer on the proposed #53276 .
- https://github.com/apache/spark/pull/53276#issuecomment-3614192738
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #52578:
URL: https://github.com/apache/spark/pull/52578#discussion_r2426134804
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PushVariantIntoScan.scala:
##########
@@ -26,9 +26,12 @@ import org.apache.spark.sql.catalyst.planning.PhysicalOperation
import org.apache.spark.sql.catalyst.plans.logical.{Filter, LogicalPlan, Project, Subquery}
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.catalyst.util.ResolveDefaultColumns
+import org.apache.spark.sql.connector.read.{SupportsPushDownVariants, VariantAccessInfo}
import org.apache.spark.sql.errors.QueryExecutionErrors
import org.apache.spark.sql.execution.datasources.parquet.ParquetFileFormat
-import org.apache.spark.sql.execution.datasources.v2.DataSourceV2Relation
+// BEGIN-V2-SUPPORT: DataSource V2 imports
Review Comment:
what does this special comment marker mean?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3397976698
I opened the revert PR https://github.com/apache/spark/pull/52590.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3398151988
I'm fine in either way, @viirya . I guess it's more up to @huaxingao 's preference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3395199785
BTW, maybe the test case is generated by `Claude Code`?
> Generated-by: Claude Code v2.0.13
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-53880][SQL] Support DSv2 in PushVariantIntoScan [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #52578:
URL: https://github.com/apache/spark/pull/52578#issuecomment-3395344743
I saw all tests are passing in last CI run but just two Java style issues. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org