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

SQL: Improve handling of invalid args for PERCENTILE/PERCENTILE_RANK #37803

Merged
merged 2 commits into from
Jan 24, 2019

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Jan 24, 2019

Improve the Exception and the error message returned when 2nd argument
of PERCENTILE and PERCENTILE_RANK is not a constant.

Improve the Exception and the error message returned when 2nd argument
of PERCENTILE and PERCENTILE_RANK is not a constant.
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left one minor nitpick.

@@ -43,8 +43,8 @@ public Percentile replaceChildren(List<Expression> newChildren) {
@Override
protected TypeResolution resolveType() {
if (!percent.foldable()) {
throw new SqlIllegalArgumentException("2nd argument of PERCENTILE must be constant, received [{}]",
Expressions.name(percent));
return new TypeResolution(format(null, "2nd argument of PERCENTILE must be constant, received [{}]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: must be A constant? Same for PERCENTILE_RANK...

@astefan astefan changed the title SQL: Improve handling of invalide args for PERCENTILE/PERCENTILE_RANK SQL: Improve handling of invalid args for PERCENTILE/PERCENTILE_RANK Jan 24, 2019
@matriv matriv merged commit 9357929 into elastic:master Jan 24, 2019
@matriv matriv deleted the mt/fix-error-msg branch January 24, 2019 13:03
matriv added a commit that referenced this pull request Jan 24, 2019
…37803)

Improve the Exception and the error message returned when 2nd argument
of PERCENTILE and PERCENTILE_RANK is not a constant.
matriv added a commit that referenced this pull request Jan 24, 2019
…37803)

Improve the Exception and the error message returned when 2nd argument
of PERCENTILE and PERCENTILE_RANK is not a constant.
matriv added a commit that referenced this pull request Jan 24, 2019
…37803)

Improve the Exception and the error message returned when 2nd argument
of PERCENTILE and PERCENTILE_RANK is not a constant.
@matriv
Copy link
Contributor Author

matriv commented Jan 24, 2019

Backported to 6.x with 1dde748
to 6.6 with 529f3ec
to 6.5 with 0d00690

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 24, 2019
* elastic/master:
  Optimize warning header de-duplication (elastic#37725)
  Bubble exceptions up in ClusterApplierService (elastic#37729)
  SQL: Improve handling of invalid args for PERCENTILE/PERCENTILE_RANK (elastic#37803)
  Remove unused ThreadBarrier class (elastic#37666)
  Add built-in user and role for code plugin (elastic#37030)
  Consolidate testclusters tests into a single project (elastic#37362)
  Fix docs for MappingUpdatedAction
  SQL: Introduce SQL DATE data type (elastic#37693)
  disabling bwc test while backporting elastic#37639
  Mute ClusterDisruptionIT testAckedIndexing
  Set acking timeout to 0 on dynamic mapping update (elastic#31140)
  Remove index audit output type (elastic#37707)
  Mute FollowerFailOverIT testReadRequestsReturnsLatestMappingVersion
  [ML] Increase close job timeout and lower the max number (elastic#37770)
  Remove Custom Listeners from SnapshotsService (elastic#37629)
  Use m_m_nodes from Zen1 master for Zen2 bootstrap (elastic#37701)
  Fix index filtering in follow info api. (elastic#37752)
  Use project dependency instead of substitutions for distributions (elastic#37730)
  Update authenticate to allow unknown fields (elastic#37713)
  Deprecate HLRC EmptyResponse used by security (elastic#37540)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants