You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2023/03/31 05:11:06 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #10515: Allow ValueBlock length to increase in TransformFunction
Jackie-Jiang opened a new pull request, #10515:
URL: https://github.com/apache/pinot/pull/10515
With chain-able ProjectOperator (introduced in #10405), `ValueBlock` documents might not be fixed. This PR modify all `TransformFunction` to allow different docs in `ValueBlock`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10515:
URL: https://github.com/apache/pinot/pull/10515#discussion_r1154111743
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/InTransformFunction.java:
##########
@@ -139,13 +139,11 @@ public TransformResultMetadata getResultMetadata() {
@Override
public int[] transformToIntValuesSV(ValueBlock valueBlock) {
int length = valueBlock.getNumDocs();
-
- if (_intValuesSV == null) {
+ if (_intValuesSV == null || _intValuesSV.length < length) {
Review Comment:
Maybe have both methods:
```
initIntValuesSV(length);
initIntValuesSV(length, defaultValue);
```
in BaseTransformFunction?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10515:
URL: https://github.com/apache/pinot/pull/10515#discussion_r1154927319
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/GroovyTransformFunction.java:
##########
@@ -429,13 +408,11 @@ public double[][] transformToDoubleValuesMV(ValueBlock valueBlock) {
@Override
public String[][] transformToStringValuesMV(ValueBlock valueBlock) {
- if (_stringValuesMV == null) {
- _stringValuesMV = new String[DocIdSetPlanNode.MAX_DOC_PER_CALL][];
- }
+ int length = valueBlock.getNumDocs();
+ initStringValuesMV(length);
Review Comment:
I'm pretty sure that is a typo (we used to always allocate 10K docs, but then changed it to length)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] 61yao commented on a diff in pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "61yao (via GitHub)" <gi...@apache.org>.
61yao commented on code in PR #10515:
URL: https://github.com/apache/pinot/pull/10515#discussion_r1154029132
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -122,12 +121,16 @@ public int[][] transformToDictIdsMV(ValueBlock valueBlock) {
throw new UnsupportedOperationException();
}
+ protected void initIntValuesSV(int length) {
+ if (_intValuesSV == null || _intValuesSV.length < length) {
Review Comment:
Do we need to do anything if _intValuesSV.length > length?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10515:
URL: https://github.com/apache/pinot/pull/10515#discussion_r1154094318
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/GroovyTransformFunction.java:
##########
@@ -429,13 +408,11 @@ public double[][] transformToDoubleValuesMV(ValueBlock valueBlock) {
@Override
public String[][] transformToStringValuesMV(ValueBlock valueBlock) {
- if (_stringValuesMV == null) {
- _stringValuesMV = new String[DocIdSetPlanNode.MAX_DOC_PER_CALL][];
- }
+ int length = valueBlock.getNumDocs();
+ initStringValuesMV(length);
Review Comment:
Why this is set as `MAX_DOC_PER_CALL` initially? Will this causing the frequent resizing problem?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10515:
URL: https://github.com/apache/pinot/pull/10515
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10515:
URL: https://github.com/apache/pinot/pull/10515#issuecomment-1491333206
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10515](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9fe0bfa) into [master](https://codecov.io/gh/apache/pinot/commit/641ec601d577d09eeb339487cd4dd7dc0441cc99?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (641ec60) will **decrease** coverage by `56.06%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10515 +/- ##
=============================================
- Coverage 69.97% 13.92% -56.06%
+ Complexity 6118 237 -5881
=============================================
Files 2092 2038 -54
Lines 112712 110098 -2614
Branches 17143 16698 -445
=============================================
- Hits 78874 15327 -63547
- Misses 28260 93518 +65258
+ Partials 5578 1253 -4325
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.92% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../transform/function/AdditionTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQWRkaXRpb25UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...nsform/function/ArrayAverageTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQXJyYXlBdmVyYWdlVHJhbnNmb3JtRnVuY3Rpb24uamF2YQ==) | `0.00% <0.00%> (-43.48%)` | :arrow_down: |
| [...ansform/function/ArrayLengthTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQXJyYXlMZW5ndGhUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-94.45%)` | :arrow_down: |
| [.../transform/function/ArrayMaxTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQXJyYXlNYXhUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-27.78%)` | :arrow_down: |
| [.../transform/function/ArrayMinTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQXJyYXlNaW5UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-27.78%)` | :arrow_down: |
| [.../transform/function/ArraySumTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQXJyYXlTdW1UcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [...ator/transform/function/BaseTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQmFzZVRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-40.70%)` | :arrow_down: |
| [...form/function/BinaryOperatorTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQmluYXJ5T3BlcmF0b3JUcmFuc2Zvcm1GdW5jdGlvbi5qYXZh) | `0.00% <0.00%> (-70.17%)` | :arrow_down: |
| [...ator/transform/function/CaseTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ2FzZVRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-69.74%)` | :arrow_down: |
| [...ator/transform/function/CastTransformFunction.java](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vQ2FzdFRyYW5zZm9ybUZ1bmN0aW9uLmphdmE=) | `0.00% <0.00%> (-57.01%)` | :arrow_down: |
| ... and [31 more](https://codecov.io/gh/apache/pinot/pull/10515?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
... and [1575 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10515/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10515: Allow ValueBlock length to increase in TransformFunction
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10515:
URL: https://github.com/apache/pinot/pull/10515#discussion_r1154925954
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BaseTransformFunction.java:
##########
@@ -122,12 +121,16 @@ public int[][] transformToDictIdsMV(ValueBlock valueBlock) {
throw new UnsupportedOperationException();
}
+ protected void initIntValuesSV(int length) {
+ if (_intValuesSV == null || _intValuesSV.length < length) {
Review Comment:
No because we use it as a reusable buffer to reduce the allocation. We'll only read up to `length` values from the buffer
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org