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/02/10 19:07:44 UTC
[GitHub] [spark] sunchao opened a new pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
sunchao opened a new pull request #35483:
URL: https://github.com/apache/spark/pull/35483
<!--
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
'core/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 changes `WritableColumnVector` to better support null structs, by avoiding allocating space in child vectors when an element is null.
### 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.
-->
Currently, for a `WritableColumnVector` whose type is struct, Spark needs to allocate space in all child vectors even if an element is null. This is not very space efficient.
In addition, this doesn't work well with the ongoing Parquet vectorized support for complex types, since when creating the child column vectors for a struct, we don't yet have the information of which slot is for null struct (Parquet packs values contiguously instead of having null slots).
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
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'.
-->
Shouldn't be, since `WritableColumnVector` is Spark internal API.
### 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.
-->
Updated `ColumnVectorSuite` and `ColumnarBatchSuite` to reflect the new 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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804201836
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -557,6 +579,9 @@ protected void reserveInternal(int newCapacity) {
Platform.reallocateMemory(lengthData, oldCapacity * 4L, newCapacity * 4L);
this.offsetData =
Platform.reallocateMemory(offsetData, oldCapacity * 4L, newCapacity * 4L);
+ } else if (isStruct()) {
+ this.structOffsetData =
+ Platform.reallocateMemory(structOffsetData, oldCapacity * 4L, newCapacity * 4L);
Review comment:
OK
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -374,6 +375,14 @@ public void putBooleans(int rowId, int count, byte src, int srcIndex) {
*/
public abstract void putArray(int rowId, int offset, int length);
+ /**
+ * Puts a new non-null struct at 'rowId' of this vector, which is backed by elements at
+ * 'offset' of child vectors.
+ *
+ * NOTE: this MUST be called after new elements are appended to child vectors of a struct vector.
+ */
+ public abstract void putStruct(int rowId, int offset);
Review comment:
Hmm, I'd prefer `putStruct`, similar to `putArray` above.
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -457,6 +466,7 @@ public final int appendNull() {
}
public final int appendNotNull() {
+ assert (!(dataType() instanceof StructType)); // Use appendStruct()
Review comment:
As mentioned in the PR description, I think it's fine since `WritableColumnVector` is a Spark internal API. Also see some [previous discussion](https://github.com/apache/spark/pull/34659#discussion_r769131591).
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -703,13 +720,13 @@ public WritableColumnVector arrayData() {
public abstract int getArrayOffset(int rowId);
- @Override
- public WritableColumnVector getChild(int ordinal) { return childColumns[ordinal]; }
-
/**
- * Returns the elements appended.
+ * Returns the offset of a struct element at 'rowId' in the child vectors of this.
*/
- public final int getElementsAppended() { return elementsAppended; }
Review comment:
Oops I removed this by accident. Seems it isn't used anywhere though.
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -374,6 +375,14 @@ public void putBooleans(int rowId, int count, byte src, int srcIndex) {
*/
public abstract void putArray(int rowId, int offset, int length);
+ /**
+ * Puts a new non-null struct at 'rowId' of this vector, which is backed by elements at
+ * 'offset' of child vectors.
Review comment:
Yea for non-null struct the offset is the same: struct[rowId] is constitute of children[i][rowId] for i in [0, struct.len).
--
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] dongjoon-hyun commented on a change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804087782
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -703,13 +720,13 @@ public WritableColumnVector arrayData() {
public abstract int getArrayOffset(int rowId);
- @Override
- public WritableColumnVector getChild(int ordinal) { return childColumns[ordinal]; }
-
/**
- * Returns the elements appended.
+ * Returns the offset of a struct element at 'rowId' in the child vectors of this.
*/
- public final int getElementsAppended() { return elementsAppended; }
+ public abstract int getStructOffset(int rowId);
+
+ @Override
+ public WritableColumnVector getChild(int ordinal) { return childColumns[ordinal]; }
Review comment:
nit. If possible, could you replace some code in order to exclude this unchanged function properly? Currently, this looks like being moved from the above to 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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804276988
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
Hm? this `reserve` is called on child columns. Then it goes to `reserveInternal`. For example, in `OnHeapColumnVector`, `reserveInternal` will re-allocate on heap array which is used to store column data. Am I missing anyplace 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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804428530
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
Hmm when `nulls[i]` is set, `structOffsetData[i]` is literally undefined since it can be any value right? the memory region is uninitialized.
--
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] timarmstrong commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
timarmstrong commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1042670516
> The problem is when creating child column vectors for a struct, we don't have the info whether a slot in the vector is for a null struct or not, as we are comparing with the child column's max definition level. The null structs are created when we are assembling the child columns. Please checkout https://github.com/apache/spark/pull/34659.
I spent about half an hour looking at that PR and I don't get why the vectorized reader needs this WritableColumnVector change. I see the code in `VectorizedRleValuesReader` is written to use this change (it only increments state.valueOffset if the parent struct is non-NULL) but I don't see why the logic needs to be like that.
This wasn't a problem in other vectorized Parquet readers I've worked with or looked at. E.g. the Apache Impala and the Apache Arrow/parquet-cpp Parquet readers read values in a spaced manner for structs without problems - https://github.com/apache/arrow/blob/master/cpp/src/parquet/column_reader.cc#L1014
> Well, this is subjective though and there is no data point proving that NULL structs is not used commonly (at least I've seen quite some of these before). Spark also reuses these vectors so the overhead is bounded by the number of projected columns.
I don't see any evidence that making this different trade-off is better though - it seems like we should have some stronger reason to think this change is a net positive given how many CPUs this code will ultimately run on. Is there some benchmark or workload where this is significantly better?
> Hm I don't get this: what complication and what conversion? as you can see the PR itself is pretty simple and it doesn't involve changes on the "rest of the engine".
I guess most of spark doesn't use these column vectors, so maybe not too important. If there is ever more efforts to add vectorized operations in Spark it would be simpler if you can apply vectorized operations to struct elements in same way as top-level elements. This maybe isn't a strong reason right now but the current Spark layout seems more future proof.
> For Arrow there is a separate code path already ArrowColumnVector. All the data eco-systems I know (e.g., RAPIDS, Intel OAP, Iceberg) are already using that. So, I'm not really convinced this is an issue.
I was thinking that people in future will want conversions between different column vector types with minimal data copying where possible, e.g. if you want to scan from parquet and call a UDF using Arrow. Producing one style of ColumnVector then converting to another is inefficient. You could avoid the copy in principle when the memory layouts of vectors are identical (pretty common for numeric types). This really isn't super-important - maybe nobody will ever want to do this this - but just another reason to not change 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
[GitHub] [spark] viirya commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804446180
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
yea, I will think "defined" in the context of `Option`-like class that you can definitely know the value is defined or not. It doesn't apply for here as it is just uninitialized so we cannot tell if a value `structOffsetData[i]` is "defined" or not by just looking at its value.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r805213950
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
looks better.
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804854322
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
I can try to revise it if it sounds confusing. How about:
> A slot 'structOffsetData[i]' will only be set iff 'nulls[i]' is NOT set. If set, 'structOffsetData[i] = j' indicates that for struct at slot 'i', its offset in the child vectors is 'j'. For those slots that are not set, their values are undefined and should never be used.
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804426890
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
I can update the PR with the `append` changes too. It seems pretty straightforward.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804417650
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
oh, it's an off-heap int array.
--
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] sunchao closed pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao closed pull request #35483:
URL: https://github.com/apache/spark/pull/35483
--
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] dongjoon-hyun commented on a change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804083074
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -457,6 +466,7 @@ public final int appendNull() {
}
public final int appendNotNull() {
+ assert (!(dataType() instanceof StructType)); // Use appendStruct()
Review comment:
Previously, we have no user to use this API for `StructType`, right? I'm wondering if this is a breaking change of this API users.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804139352
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -374,6 +375,14 @@ public void putBooleans(int rowId, int count, byte src, int srcIndex) {
*/
public abstract void putArray(int rowId, int offset, int length);
+ /**
+ * Puts a new non-null struct at 'rowId' of this vector, which is backed by elements at
+ * 'offset' of child vectors.
+ *
+ * NOTE: this MUST be called after new elements are appended to child vectors of a struct vector.
+ */
+ public abstract void putStruct(int rowId, int offset);
Review comment:
`putStructOffset`? To be pair with `getStructOffset`?
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804425518
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
Yes, for `append` it still add null in child vectors, for null struct.
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804239260
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
}
}
} else {
- appendNotNull();
+ putNotNull(elementsAppended);
}
+ putStruct(elementsAppended, elementsAppended);
Review comment:
For the `isNull` case, clients should just call `putNull` or `putNulls`. I think this is the same as other types like int, boolean, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804439406
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
Sure, updated.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804138119
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -374,6 +375,14 @@ public void putBooleans(int rowId, int count, byte src, int srcIndex) {
*/
public abstract void putArray(int rowId, int offset, int length);
+ /**
+ * Puts a new non-null struct at 'rowId' of this vector, which is backed by elements at
+ * 'offset' of child vectors.
Review comment:
If there is null element in any child vector? The offset is still not changed?
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804421765
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
To confirm: now we have two ways to construct writable column vectors (`append` or `put`), which can result to column vectors representing same values but with different memory layout (null struct is stored differently)?
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804422405
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
actually "defined" is a bit confusing when I first read this, because it doesn't look like we set it in "undefined" value. I guess its meaning is, if the index i is null at this column vector, then the index of `structOffsetData` is meaningful and will/can be used.
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804428530
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
Hmm when `nulls[I]` is set, `structOffsetData[i]` is literally undefined since it can be any value right? the memory region is uninitialized.
--
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] sunchao commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1040613408
gently ping @cloud-fan - could you take another look? :)
--
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] sunchao commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1058390811
Took a while to get back to this PR: I investigated more on why I needed this change originally from Parquet side, and turned out it's for handling cases like array of structs, and it seems there is a separate solution to handle the case. Therefore, to make it less disruptive, I'll close this PR for now.
Thanks all for taking a look!
--
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] dongjoon-hyun commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1058396543
Thank you for the decision, @sunchao .
--
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] dongjoon-hyun commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1042007876
Gentle ping, @cloud-fan .
--
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] timarmstrong commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
timarmstrong commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1042349079
Hi,
I found out about this change because I work on some things that depends on the memory layout of `OffHeapColumnVector`s and assumes that data for struct columns has the same layout as top-level columns (i.e. storage is allocated for NULL values).
I'm biased, but I have a few reasons I think this is not the right direction for Spark:
* Other users and tools might be depending on the memory layout of `{On,Off}HeapColumnVector` since it's possible to access the underlying memory directly. I.e. this could cause breakage to user code. I don't know whether we consider this part of the public API or not.
* This adds memory and CPU overhead for a common case (non-NULL struct) to optimize for a different case (NULL structs). I don't have data to prove that NULL structs are queried less frequently than NULL structs but it seems likely. It doesn't help with worst-case memory consumption either.
* This makes the memory layout for top-level fields inconsistent with memory layout for fields inside structs, which can complicate the rest of the engine (need separate code for top-level column vectors and nested column vectors).
* Unnesting a struct field, e.g. projecting `SELECT x.y as x` would require a conversion of the value data because the memory layout is different.
* The current memory layout is more consistent with Arrow - https://arrow.apache.org/docs/format/Columnar.html#struct-layout - so as more of the data ecosystem moves towards Arrow as the interchange format, we're locked into doing data format conversions.
> In addition, this doesn't work well with the ongoing Parquet vectorized support for complex types, since when creating the child column vectors for a struct, we don't yet have the information of which slot is for null struct (Parquet packs values contiguously instead of having null slots).
As someone who's fairly familiar with the Parquet spec and has worked on a couple of Parquet readers, this motivation seems incomplete. Data for top-level fields in Parquet is stored in exactly the same way as nested fields. Why does the approach used to scatter top-level values to the output vector not work for structs? Conceptually it should be the same algorithm except you are scattering values with `def_level >= field_def_level` instead of `def_level >= 1`. Something like:
```
for (int i = 0; i < num_values; i++) {
bool is_null = parquet_def_levels[i] >= field_def_level
out.nulls[i] = is_null
if (!is_null) {
out.values[i] = parquet_values[curr_value_idx]
curr_value_idx++
}
}
```
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804211605
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -703,13 +720,13 @@ public WritableColumnVector arrayData() {
public abstract int getArrayOffset(int rowId);
- @Override
- public WritableColumnVector getChild(int ordinal) { return childColumns[ordinal]; }
-
/**
- * Returns the elements appended.
+ * Returns the offset of a struct element at 'rowId' in the child vectors of this.
*/
- public final int getElementsAppended() { return elementsAppended; }
+ public abstract int getStructOffset(int rowId);
+
+ @Override
+ public WritableColumnVector getChild(int ordinal) { return childColumns[ordinal]; }
Review comment:
I think it's better now after revive `getElementsAppended`
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804235091
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
}
}
} else {
- appendNotNull();
+ putNotNull(elementsAppended);
}
+ putStruct(elementsAppended, elementsAppended);
Review comment:
For `isNull` case, we also need do `putStruct`?
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r805213950
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
looks better.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804267367
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
Hmm, I think the main purpose of this PR is to "avoiding allocating space in child vectors when an element is null." Here we still call `appendNull` which does `reserve` that will allocate more space if needed. So I am not sure if we avoid allocating space? Or it is in other place I missed?
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804299182
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
Oh I see. I think this PR doesn't change the behavior of `append` - it only changes the `put` API which is the main target and will be used by the Parquet work. I remember I wanted to minimize the impact, since `append` is used in columnar-to-row transition.
I can have a follow-up PR to change `append` too if that's useful.
--
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] dongjoon-hyun commented on a change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804086469
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -703,13 +720,13 @@ public WritableColumnVector arrayData() {
public abstract int getArrayOffset(int rowId);
- @Override
- public WritableColumnVector getChild(int ordinal) { return childColumns[ordinal]; }
-
/**
- * Returns the elements appended.
+ * Returns the offset of a struct element at 'rowId' in the child vectors of this.
*/
- public final int getElementsAppended() { return elementsAppended; }
Review comment:
Do we need to remove this API inevitably? To be safe, I hope we undo this removal from this `Improve` PR (if possible).
When we start to remove somethings, the review becomes tricky.
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804242349
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
}
}
} else {
- appendNotNull();
+ putNotNull(elementsAppended);
}
+ putStruct(elementsAppended, elementsAppended);
Review comment:
Oops sorry I misunderstood you. Yes I think we can move this to the not-null branch. for `isNull` case it is unnecessary.
--
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] timarmstrong commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
timarmstrong commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1065300491
@sunchao great to hear!
--
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] sunchao commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1042384416
> Other users and tools might be depending on the memory layout of {On,Off}HeapColumnVector since it's possible to access the underlying memory directly. I.e. this could cause breakage to user code. I don't know whether we consider this part of the public API or not.
As mentioned in the PR description, these are not public APIs and hence the change.
> This adds memory and CPU overhead for a common case (non-NULL struct) to optimize for a different case (NULL structs). I don't have data to prove that NULL structs are queried less frequently than NULL structs but it seems likely. It doesn't help with worst-case memory consumption either.
Well, this is subjective though and there is no data point proving that NULL structs is _not_ used commonly (at least I've seen quite some of these before). Spark also reuses these vectors so the overhead is bounded by the number of projected columns.
> This makes the memory layout for top-level fields inconsistent with memory layout for fields inside structs, which can complicate the rest of the engine (need separate code for top-level column vectors and nested column vectors).
Unnesting a struct field, e.g. projecting SELECT x.y as x would require a conversion of the value data because the memory layout is different.
Hm I don't get this: what complication and what conversion? as you can see the PR itself is pretty simple and it doesn't involve changes on the "rest of the engine".
> The current memory layout is more consistent with Arrow - https://arrow.apache.org/docs/format/Columnar.html#struct-layout - so as more of the data ecosystem moves towards Arrow as the interchange format, we're locked into doing data format conversions.
For Arrow there is a separate code path already `ArrowColumnVector`. All the data eco-systems I know (e.g., RAPIDS, Intel OAP, Iceberg) are already using that. So, I'm not really convinced this is an issue.
> As someone who's fairly familiar with the Parquet spec and has worked on a couple of Parquet readers, this motivation seems incomplete. Data for top-level fields in Parquet is stored in exactly the same way as nested fields. Why does the approach used to scatter top-level values to the output vector not work for structs? Conceptually it should be the same algorithm except you are scattering values with def_level >= field_def_level instead of def_level >= 1. Something like
The problem is when creating child column vectors for a struct, we don't have the info whether a slot in the vector is for a null struct or not, as we are comparing with the child column's max definition level. The null structs are created when we are assembling the child columns. Please checkout #34659.
--
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] sunchao commented on a change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804271643
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
`reserve` doesn't affect child columns though, it only affects `null` and `offset` array.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804417269
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -64,13 +64,23 @@
private long lengthData;
private long offsetData;
+ // Only set if type is Struct.
+ //
+ // A slot 'structOffsetData[i]' is only defined iff 'nulls[i]' is NOT set. If defined,
Review comment:
This comment is a bit confusing. `structOffsetData` is a long and what does `structOffsetData[i]` 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
[GitHub] [spark] viirya commented on a change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804137594
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -374,6 +375,14 @@ public void putBooleans(int rowId, int count, byte src, int srcIndex) {
*/
public abstract void putArray(int rowId, int offset, int length);
+ /**
+ * Puts a new non-null struct at 'rowId' of this vector, which is backed by elements at
+ * 'offset' of child vectors.
Review comment:
The offsets for all child vectors are the same for the non-null struct?
--
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] dongjoon-hyun commented on a change in pull request #35483: [SPARK-38179][SQL] Improve WritableColumnVector to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804075773
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java
##########
@@ -557,6 +579,9 @@ protected void reserveInternal(int newCapacity) {
Platform.reallocateMemory(lengthData, oldCapacity * 4L, newCapacity * 4L);
this.offsetData =
Platform.reallocateMemory(offsetData, oldCapacity * 4L, newCapacity * 4L);
+ } else if (isStruct()) {
+ this.structOffsetData =
+ Platform.reallocateMemory(structOffsetData, oldCapacity * 4L, newCapacity * 4L);
Review comment:
Two more spaces like the above lines.
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804235690
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
}
}
} else {
- appendNotNull();
+ putNotNull(elementsAppended);
}
+ putStruct(elementsAppended, elementsAppended);
Review comment:
I think the doc of `putStruct` is `Puts a new non-null struct ...`
--
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 change in pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35483:
URL: https://github.com/apache/spark/pull/35483#discussion_r804437630
##########
File path: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java
##########
@@ -661,21 +671,22 @@ public final int appendArray(int length) {
* common non-struct case.
*/
public final int appendStruct(boolean isNull) {
+ reserve(elementsAppended + 1);
if (isNull) {
// This is the same as appendNull but without the assertion for struct types
- reserve(elementsAppended + 1);
putNull(elementsAppended);
- elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType) {
+ if (c.isStruct()) {
c.appendStruct(true);
} else {
c.appendNull();
Review comment:
Yea let's update `append` as well. I think it's better if the `ColumVector` data format is normalized: we only have one way to represent the same values.
--
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] dongjoon-hyun commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1037551504
Could you review this once more on the last commit, @cloud-fan ?
--
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] dongjoon-hyun commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1037551504
Could you review this once more on the last commit, @cloud-fan ?
--
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] sunchao commented on pull request #35483: [SPARK-38179][SQL] Improve `WritableColumnVector` to better support null struct
Posted by GitBox <gi...@apache.org>.
sunchao commented on pull request #35483:
URL: https://github.com/apache/spark/pull/35483#issuecomment-1040613408
gently ping @cloud-fan - could you take another look? :)
--
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