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.

Sign up for GitHub

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

Jump to bottom

St_* function error messages + support literal transform functions #8001

Merged
merged 2 commits into from
Feb 5, 2022
Merged

St_* function error messages + support literal transform functions #8001

merged 2 commits into from
Feb 5, 2022

Conversation

mneedham
Copy link
Contributor

@mneedham mneedham commented Jan 12, 2022 •
edited
Loading

Description

  • Updates the error message returned by St_Contains, St_Area, St_Distance, and St_Equals to indicate to the user what argument type they actually passed to the function.
  • St_Equals and St_Area accept literal transform functions (the same as the other St_ functions)

The change to St_Equals lets us compare the location in a column against a literal location:

image

And the change to St_Area does the same thing:

image

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

  • Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

  • Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

  • New configuration options
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

@mneedham mneedham changed the title Update error message to indicate the actual type + allow literal transform functions in StContainsFunction Update error message to indicate the actual type + allow literal transform functions in StEqualsFunction Jan 12, 2022
@mneedham mneedham changed the title Update error message to indicate the actual type + allow literal transform functions in StEqualsFunction St_* function error messages + support literal transform functions Jan 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@e59730a). Click here to learn what that means.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8001   +/-   ##
=========================================
  Coverage          ?   71.40%           
  Complexity        ?     4303           
=========================================
  Files             ?     1624           
  Lines             ?    84152           
  Branches          ?    12597           
=========================================
  Hits              ?    60092           
  Misses            ?    19955           
  Partials          ?     4105
Flag Coverage Δ
integration1 28.93% <0.00%> (?)
integration2 27.63% <0.00%> (?)
unittests1 67.95% <76.92%> (?)
unittests2 14.16% <0.00%> (?)

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

Impacted Files Coverage Δ
.../geospatial/transform/function/StAreaFunction.java 84.00% <50.00%> (ø)
...eospatial/transform/function/StEqualsFunction.java 82.14% <60.00%> (ø)
...spatial/transform/function/StContainsFunction.java 75.86% <100.00%> (ø)
...spatial/transform/function/StDistanceFunction.java 84.48% <100.00%> (ø)
...eospatial/transform/function/StWithinFunction.java 75.86% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e59730a...07b9492. Read the comment docs.

walterddr
walterddr reviewed Jan 14, 2022
View reviewed changes
...t-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StAreaFunction.java Outdated
Preconditions.checkArgument(transformFunction.getResultMetadata().getDataType() == FieldSpec.DataType.BYTES,
"The argument must be of bytes type");
Preconditions.checkArgument(transformFunction.getResultMetadata().getDataType() == FieldSpec.DataType.BYTES
|| transformFunction instanceof LiteralTransformFunction, notBytesErrorMessage(transformFunction));
Copy link
Contributor

Choose a reason for hiding this comment

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

i am not sure this logic is correct. this means if transformFunction is Literal, you don't check for the DataType of the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@walterddr yeh this is a good question! But all the existing ST_* functions do it like this and it seems to work alright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@walterddr see an example here of what I mean - https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StContainsFunction.java#L60

Copy link
Contributor

@walterddr walterddr Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ahh. this is the reason. nvm then.

pinot/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/LiteralTransformFunction.java

Line 39 in ca08968

* TODO: Preserve the type of the literal instead of inferring the type from the string

@mneedham mneedham force-pushed the st-argument-type-error-message branch from 88a2f5b to 60aa2d4 Compare January 17, 2022 15:13
richardstartin
richardstartin approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@richardstartin richardstartin left a comment

Choose a reason for hiding this comment

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

👍

yupeng9
yupeng9 reviewed Jan 18, 2022
View reviewed changes
...core/src/main/java/org/apache/pinot/core/geospatial/transform/function/StEqualsFunction.java Outdated
Preconditions.checkArgument(transformFunction.getResultMetadata().getDataType() == FieldSpec.DataType.BYTES
|| transformFunction instanceof LiteralTransformFunction,
String.format("The %s argument must be of type %s , but was %s",
"first",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, you can inline first in the msg

Copy link
Member

Choose a reason for hiding this comment

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

This is worthwhile because it avoids the varargs overload too

@mneedham mneedham force-pushed the st-argument-type-error-message branch from c077ad5 to 07b9492 Compare February 4, 2022 09:29
@richardstartin
Copy link
Member

@Jackie-Jiang @yupeng9 any objections to merging this?

yupeng9
yupeng9 approved these changes Feb 4, 2022
View reviewed changes
Copy link
Contributor

@yupeng9 yupeng9 left a comment

Choose a reason for hiding this comment

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

LGTM

@Jackie-Jiang Jackie-Jiang merged commit ede1882 into apache:master Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterddr walterddr walterddr left review comments

@richardstartin richardstartin richardstartin approved these changes

@yupeng9 yupeng9 yupeng9 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mneedham @codecov-commenter @richardstartin @walterddr @yupeng9 @Jackie-Jiang