Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[multistage] add test for LiteralValueOperator #9796

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Nov 14, 2022

Super quick and easy test :)

@codecov-commenter
Copy link

Codecov Report

Merging #9796 (910b2b7) into master (daab972) will decrease coverage by 44.97%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master    #9796       +/-   ##
=============================================
- Coverage     70.17%   25.20%   -44.98%     
+ Complexity     5407       44     -5363     
=============================================
  Files          1956     1944       -12     
  Lines        104975   104622      -353     
  Branches      15892    15854       -38     
=============================================
- Hits          73669    26369    -47300     
- Misses        26157    75546    +49389     
+ Partials       5149     2707     -2442     
Flag Coverage Δ
integration1 25.20% <ø> (-0.08%) ⬇️
integration2 ?
unittests1 ?
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...in/java/org/apache/pinot/spi/utils/BytesUtils.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/table/FSTType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/config/user/RoleType.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/data/MetricFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/apache/pinot/spi/stream/StreamMessage.java 0.00% <0.00%> (-100.00%) ⬇️
...java/org/apache/pinot/common/tier/TierFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/spi/config/table/TableType.java 0.00% <0.00%> (-100.00%) ⬇️
.../org/apache/pinot/spi/data/DimensionFieldSpec.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1418 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@siddharthteotia siddharthteotia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be we also want to add a simple test for input null / empty and the operator shouldn't NPE or anything, return empty.

@siddharthteotia siddharthteotia added the multi-stage Related to the multi-stage query engine label Nov 14, 2022
// Then:
Assert.assertEquals(transferableBlock.getContainer().get(0), new Object[]{"foo", 1});
Assert.assertEquals(transferableBlock.getContainer().get(1), new Object[]{"bar", 2});
Assert.assertTrue(operator.nextBlock().isEndOfStreamBlock(), "Expected EOS after reading two rows");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also add a test for literals that are empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added, but how can you create an empty literal in first place (in SQL)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SELECT col1 FROM tbl WHERE col2 BETWEEN 0 AND -1 will generate an empty literal value node with schema of col1 and 0 rows.

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. but also could you add test for empty literals?

@walterddr walterddr merged commit 47c8f18 into apache:master Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multi-stage Related to the multi-stage query engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants