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

SCRIPTING: Support BucketAggScript return null #32811

Merged
merged 1 commit into from
Aug 13, 2018
Merged

SCRIPTING: Support BucketAggScript return null #32811

merged 1 commit into from
Aug 13, 2018

Conversation

original-brownbear
Copy link
Member

@original-brownbear
SCRIPTING: Support BucketAggScript return null
ae9fb65 
* As explained in elastic#32790, `BucketAggregationScript` must support `null` as a return value
* Closes elastic#32790
@original-brownbear original-brownbear added >bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v7.0.0 v6.5.0 labels Aug 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@original-brownbear original-brownbear mentioned this pull request Aug 13, 2018
Closed
rjernst
rjernst reviewed Aug 13, 2018
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks good but one question. Also, this can be marked as a non-issue instead of bug since it was never released.

.../elasticsearch/search/aggregations/pipeline/bucketscript/BucketScriptPipelineAggregator.java
if (returned == null) {
newBuckets.add(bucket);
} else {
final List<InternalAggregation> aggs = StreamSupport.stream(bucket.getAggregations().spliterator(), false).map(
Copy link
Member

Choose a reason for hiding this comment

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

Should this else clause be returned to what it was before the original refactoring PR?
https://github.com/elastic/elasticsearch/pull/32068/files#diff-c94184ea4ef180f10817aa2bbd41a8edL119

Copy link
Member Author

@original-brownbear original-brownbear Aug 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@rjernst I don't think that's necessary since we're now returning a Double or null, so no need for the instanceof (which appears to be the only difference) check there right?

Copy link
Member

Choose a reason for hiding this comment

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

Ok I see. Makes sense.

@original-brownbear
Copy link
Member Author

@rjernst thanks, is this good to merge once green?

rjernst
rjernst approved these changes Aug 13, 2018
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Yes, LGTM.

@original-brownbear
Copy link
Member Author

@rjernst thanks!

@original-brownbear original-brownbear merged commit d412230 into elastic:master Aug 13, 2018
@original-brownbear original-brownbear deleted the 32790 branch August 13, 2018 18:08
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Aug 14, 2018
@original-brownbear
SCRIPTING: Support BucketAggScript return null (elastic#32811)
125ad89 
* As explained in elastic#32790, `BucketAggregationScript` must support `null` as a return value
* Closes elastic#32790
original-brownbear added a commit that referenced this pull request Aug 14, 2018
@original-brownbear
SCRIPTING: Support BucketAggScript return null (#32811) (#32833)
8135672 
* As explained in #32790, `BucketAggregationScript` must support `null` as a return value
* Closes #32790
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into allowed-index-stor…
ec4ab87 
…e-types

* elastic/master: (199 commits)
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 14, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into global-checkpoint-…
aad650a 
…listeners

* elastic/master:
  Watcher: Remove unused hipchat render method (elastic#32211)
  Watcher: Remove extraneous auth classes (elastic#32300)
  Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285)
  [TEST] Select free port for Minio (elastic#32837)
  MINOR: Remove `IndexTemplateFilter` (elastic#32841)
  Core: Add java time version of rounding classes (elastic#32641)
  Aggregations/HL Rest client fix: missing scores (elastic#32774)
  HLRC: Add Delete License API (elastic#32586)
  INGEST: Create Index Before Pipeline Execute (elastic#32786)
  Fix NOOP bulk updates (elastic#32819)
  Remove client connections from TcpTransport (elastic#31886)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  Mute security-cli tests in FIPS JVM (elastic#32812)
  SCRIPTING: Support BucketAggScript return null (elastic#32811)
  Unmute WildFly tests in FIPS JVM (elastic#32814)
  [TEST] Force a stop to save rollup state before continuing (elastic#32787)
  [test] disable packaging tests for suse boxes
  Mute IndicesRequestIT#testBulk
  [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)
jasontedor added a commit that referenced this pull request Aug 15, 2018
@jasontedor
Merge branch '6.x' into ccr-6.x
53cee21 
* 6.x: (96 commits)
  Introduce global checkpoint listeners (#32696)
  Use JDK 10 for 6.4 BWC builds (#32866)
  Remove unused imports - follow up to removal of test in issue 32855
  Removed flaky test. Looks like randomisation makes these assertions unreliable. This test is superfluous - it was added to address #32770 but it later turned out there was an existing test that just required a fix to provide the missing test coverage.
  [test] mute IndexShardTests.testDocStats
  Test: Fix forbidden uses in test framework (#32824)
  Security: remove password hash bootstrap check (#32440)
  Move validation to server for put user requests (#32471)
  [ML] Add high level REST client docs for ML put job endpoint (#32843)
  Painless: Change fqn_only to no_import (#32817)
  [test] mute testSearchWithSignificantTermsAgg
  Backport: CompletableContext class to avoid throwable (#32829)
  [TEST] Select free port for Minio (#32837)
  SCRIPTING: Support BucketAggScript return null (#32811) (#32833)
  HLRC: Add Delete License API (#32586)
  Aggregations/HL Rest client fix: missing scores (#32774)
  HLRC: migration get assistance API (#32744)
  Fix NOOP bulk updates (#32819)
  Increase logging testRetentionPolicyChangeDuringRecovery
  AwaitsFix case-functions.sql-spec
  ...
@davidkyle
Copy link
Member

Thanks for fixing this @original-brownbear much ❤️ from the ml team

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >non-issue v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@original-brownbear @elasticmachine @davidkyle @rjernst @jimczi