Fix node reduction pushdown tests for release tests#139548
Fix node reduction pushdown tests for release tests#139548carlosdelest merged 8 commits intoelastic:mainfrom
Conversation
carlosdelest
added
>test
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
alex-spies
left a comment
alex-spies
left a comment
There was a problem hiding this comment.
Looks good, but maybe we can future-proof the tests for when the reduction gets released.
| } | ||
|
|
||
| public void testReductionPlanForTopNWithPushedDownFunctions() { | ||
| assumeTrue("Reduction is just enabled for snapshot builds", Build.current().isSnapshot()); |
There was a problem hiding this comment.
Is there no capability that would correspond to this?
Without a capability, we're very likely to forget to update this test later to remove this assumeTrue.
There was a problem hiding this comment.
There is none AFAIU. We're using query pragmas for node reduction.
Is there an alternative for detecting that?
There was a problem hiding this comment.
I see the pragma defaults to true. Doesn't it mean it should be active here by default?
There was a problem hiding this comment.
I don't think we did this before, but I was going to suggest that maybe we could change the snapshot check with a pragma check. Just to have a link between the feature and the test ignore.
The worst part of the "isSnapshot()" is that they're "hard" to find. Meanwhile, with a capability (Or a pragma), it's easier to trackfollowing references.
Of course, if it defaults to true, it wouldn't solve anything
There was a problem hiding this comment.
The pragma defaults to true - but I don't see them being active on release builds.
Happy to include a different check, I'm just not sure on how to do it on a non-fragile way.
There was a problem hiding this comment.
I see the pragma defaults to true. Doesn't it mean it should be active here by default?
++. I think the late materialization in case of topn may still be turned off on snapshot - I thought we had a capability for that but couldn't find it.
@GalLalouche , we discussed that we wanted to disable this on snapshot builds, but I don't recall which mechanism we settled on to achieve this.
There was a problem hiding this comment.
@GalLalouche @alex-spies I'd like to move this forward. Should we change the query pragma for a snapshot capability? Something else?
There was a problem hiding this comment.
It is indeed disabled for snapshot builds: https://github.com/elastic/elasticsearch/blob/main/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeComputeHandler.java#L195. It also depends on the aforementioned pragma, but there is no ad-hoc pragma just for Top N late materialization.
There was a problem hiding this comment.
Ah I see. Should we then change the dependency on snapshot builds for a Capability (enabled on snsapshots for now) so we can check for it on tests then?
…ts-local-optimizer # Conflicts: # muted-tests.yml
alex-spies
left a comment
alex-spies
left a comment
There was a problem hiding this comment.
Thanks @carlosdelest , I think it's easier to track corresponding tests with the new capability.
@GalLalouche , are there other tests that were disable based simply on Build.current().isSnapshot()? If so, can we tie them to the new capability as well?
@alex-spies @carlosdelest There's |
…cal-optimizer' into tests/fix-release-tests-local-optimizer
Nice, done in 8c90d18. |
* upstream/main: (253 commits) Adds ST_SIMPLIFY geo spatial function (elastic#136309) Take control of max clause count verification in Lucene searcher (elastic#139752) [ML] Unmute Inference Test (elastic#139765) Parameterize the vector operation benchmark tests (elastic#139735) Fix node reduction pushdown tests for release tests (elastic#139548) Fix flakiness in TSDataGenerationHelper (elastic#139759) CPS: Copy existing resolved index expressions when constructing a new `SearchRequest` from an existing one (elastic#139596) Add release notes for v9.1.9 release (elastic#139674) Add lucene query for wildcards on high cardinality keyword fields. (elastic#139746) Suppress Tika entitlement warnings from AWT (elastic#139711) Check field storage when synthetic source is enabled, in tests (elastic#139715) Refactor VectorSimilarityType to know about its corresponding Function (elastic#139678) Merge fixes from patch branch back into main (elastic#139721) Define native bulk operations for vector square distance (elastic#139198) Use LongUpDownCounter for Linked Project Error Metrics (elastic#139657) ESQL: Add javadoc that explains version-aware planning (elastic#139706) Add helper to pick node for reindex relocation (elastic#139081) Fix auth serialization randomized version test (elastic#139182) ES|QL - Add parsing, preanalysis and analysis timing information to profile (elastic#139540) Mute org.elasticsearch.persistent.ClusterPersistentTasksCustomMetadataTests testMinVersionSerialization elastic#139741 ...
5 participants
Closes #139490
Closes #139491
Closes #139492
Closes #139493
Node reduction only happens on snapshot builds. Add guard on the corresponding optimization tests so they check they're on snapshot builds.