[SPARK-50463][SQL] Fix ConstantColumnVector with Columnar to Row conversion
#49021
Conversation
richardc-db
changed the title
|
hey @viirya mind taking a quick look at this one? Fixes using |
| @@ -77,6 +77,11 @@ public ConstantColumnVector(int numRows, DataType type) { | |||
| } | |||
| } | |||
|
|
|||
| public void closeIfFreeable() { | |||
There was a problem hiding this comment.
ah yeah my apologies - do you think closeIfNotWritable still makes sense given that the ConstantColumnVector is not a WritableColumnVector?
Do you think closeIfFreeable makes sense? If so, I'll change the method name in ColumnVector.java in this PR as well.
Or do you have a preference for a different name?
There was a problem hiding this comment.
Ok for closeIfFreeable. I just wondered why you only changed in ConstantColumnVector.
richardc-db
force-pushed
the
col_to_row_const_col_vec_fix
branch
from
3ee4f29 to
9e453bf
Compare
sql/catalyst/src/main/java/org/apache/spark/sql/vectorized/ColumnVector.java
Outdated
Show resolved
Hide resolved
| public void closeIfNotWritable() { | ||
| // By default, we just call close() for all column vectors. If a column vector is writable, it | ||
| // should override this method and do nothing. | ||
| public void closeIfFreeable() { |
There was a problem hiding this comment.
Hmm ColumnVector is an API actually. Can we avoid direct renaming here
There was a problem hiding this comment.
The original PR which added this method 800faf0 was merged ~a month ago. is it too late to now change the method name?
If so, how do you suggest we go about this? I think closeIfNotWritable doesn't represent the purpose of the method well, so one option is to deprecate closeIfNotWritable and directly call closeIfFreeable inside it. This is pretty ugly, though, so I'm open to any suggestions
There was a problem hiding this comment.
ah okie dokie then should be fine
There was a problem hiding this comment.
Yea, it is a new API just added recently and not released yet.
There was a problem hiding this comment.
+1, LGTM. Thank you, @richardc-db , @viirya , @HyukjinKwon .
Merged to master for Apache Spark 4.0.0 on February 2025.
dongjoon-hyun
changed the title
ConstantColumnVector with Columnar to Row conversion
…nversion ### What changes were proposed in this pull request? apache@800faf0 frees column vector resources between batches in columnar to row conversion. However, like `WritableColumnVector`, `ConstantColumnVector` should not free resources between batches because the same data is used across batches ### Why are the changes needed? Without this change, ConstantColumnVectors with string values, for example, will fail if used with column->row conversion. For instance, reading a parquet table partitioned by a string column with multiple batches. ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? added UT that failed before and now passes ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#49021 from richardc-db/col_to_row_const_col_vec_fix. Authored-by: Richard Chen <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
|
@dongjoon-hyun I found that the previous PR of this one, which is #48767, was merged into both master and branch-3.5, but this pr was only merged into master. Then I manually copied the new test cases from this pr and found that they fail in branch-3.5. So should we backport this patch to branch-3.5? I have created a backporting: #49131 |
|
Oh, is this a regression then, @LuciferYang ? |
I think so... |
What changes were proposed in this pull request?
800faf0 frees column vector resources between batches in columnar to row conversion. However, like
WritableColumnVector,ConstantColumnVectorshould not free resources between batches because the same data is used across batchesWhy are the changes needed?
Without this change, ConstantColumnVectors with string values, for example, will fail if used with column->row conversion. For instance, reading a parquet table partitioned by a string column with multiple batches.
Does this PR introduce any user-facing change?
How was this patch tested?
added UT that failed before and now passes
Was this patch authored or co-authored using generative AI tooling?
no