hemanthumashankar0511 commented on code in PR #6853:
URL: https://github.com/apache/hive/pull/6853#discussion_r4409210994
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ptf/VectorPTFOperator.java:
##########
@@ -752,30 +762,46 @@ private void setCurrentPartition(VectorizedRowBatch
batch) {
System.arraycopy(bytes, start, currentPartitionByteArrays[i], 0,
length);
}
currentPartitionByteLengths[i] = length;
+ currentPartitionValues[i] =
Arrays.copyOf(currentPartitionByteArrays[i], length);
Review Comment:
it looks like `currentPartitionValues` gets filled on every partition
change, including copying the bytes for string keys, even when no evaluator
uses it. Would it be worth skipping this when no evaluator needs the
partition-only value? Totally fine if the cost is too small to matter.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ptf/VectorPTFEvaluatorBase.java:
##########
@@ -228,4 +229,29 @@ public void addStreamingGroupResults(List<Integer>
groupRowCounts) throws HiveEx
throw new HiveException(
"No streaming group result precomputation for " +
this.getClass().getName());
}
+
+ public void enablePartitionOnlyConstant() {
+ usePartitionOnlyConstant = true;
+ }
+
+ protected boolean getUsePartitionOnlyConstant() {
Review Comment:
I couldn't find any callers of `getUsePartitionOnlyConstant().` Is it needed?
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ptf/VectorPTFEvaluatorDecimalSum.java:
##########
@@ -45,6 +46,22 @@ public VectorPTFEvaluatorDecimalSum(WindowFrameDef
windowFrameDef, VectorExpress
resetEvaluator();
}
+ @Override
+ protected void accumulatePartitionOnlyBatch(int batchSize) throws
HiveException {
+ if(partitionOnlyConstantValue == null){
Review Comment:
tiny formatting thing. if(...){ should be if (...) {
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ptf/VectorPTFEvaluatorDecimalSum.java:
##########
@@ -59,6 +76,10 @@ public void evaluateGroupBatch(VectorizedRowBatch batch)
if (size == 0) {
return;
}
+ if(usePartitionOnlyConstant){
Review Comment:
tiny formatting thing. if(...){ should be if (...) {
##########
ql/src/test/queries/clientpositive/vector_ptf_unbuffered_partition_col.q:
##########
Review Comment:
The PR updates the Long, Double and Decimal sum evaluators, but if I'm
reading the tests right, only the double path (c_float) runs. Could we add a
case like `sum(c_int) over(partition by c_int order by key)` for the long path,
and maybe one with a decimal column too?
##########
ql/src/test/queries/clientpositive/vector_ptf_unbuffered_partition_col.q:
##########
@@ -16,7 +16,7 @@ from cbo_t1 order by key, s;
select key, sum(c_float) over(partition by c_float, key order by c_float,
value) as s
from cbo_t1 order by key, s;
--- Case 3: partition-only column referenced directly as a column argument
(row-mode fallback).
+-- Case 3: partition-only column referenced directly as a column argument
(vectorized).
Review Comment:
I noticed Case 3 used to be the test that covered the row-mode fallback from
#6752 . Now that it's vectorized, I couldn't find any other test that still
produces the "partition-only column not buffered" message, but the fallback
still applies to functions other than` SUM`. Would it make sense to add a small
case like `avg(c_float) over(partition by c_float order by key)` so the
fallback stays covered?
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/vector/ptf/VectorPTFEvaluatorDoubleSum.java:
##########
@@ -39,6 +39,20 @@ public VectorPTFEvaluatorDoubleSum(WindowFrameDef
windowFrameDef, VectorExpressi
resetEvaluator();
}
+ @Override
+ protected void accumulatePartitionOnlyBatch(int batchSize) {
+ if(partitionOnlyConstantValue == null){
Review Comment:
tiny formatting thing. if(...){ should be if (...) {
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
