-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Cleanup esql request building api #138398
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
Cleanup esql request building api #138398
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
luigidellaquila
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it!
We could probably consider to backport it, if it's not too much effort, to avoid conflicts in the future (the change is simple, but it impacts many files)
ivancea
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
+1 to backporting, if not too troublesome
| * For secret access to ES|QL internals only. Do not use. | ||
| * TODO qualify export when ES|QL is modularized | ||
| */ | ||
| public class SharedSecrets { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was only used in EsqlQueryRequestBuilder. I am not convinced we need it.
...ternalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClusterUsageTelemetryIT.java
Outdated
Show resolved
Hide resolved
bpintea
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💔 Backport failed
You can use sqren/backport to manually backport by running |
(cherry picked from commit daab115)
(cherry picked from commit daab115)
|
Opened a 9.2 backport. |
…-json * upstream/main: (247 commits) Mute org.elasticsearch.xpack.inference.integration.SemanticTextIndexOptionsIT testValidateIndexOptionsWithBasicLicense elastic#138513 Mute org.elasticsearch.xpack.esql.heap_attack.HeapAttackLookupJoinIT testLookupExplosionBigString elastic#138510 This shouldn't be zero (elastic#138501) sum of empty histogram is now null (elastic#138378) Test ES|QL bfloat16 support (elastic#138499) Fix exception handling in S3 `compareAndExchangeRegister` (elastic#138488) Mute org.elasticsearch.xpack.exponentialhistogram.ExponentialHistogramFieldMapperTests testFormattedDocValues elastic#138504 Mute org.elasticsearch.ingest.geoip.IngestGeoIpClientYamlTestSuiteIT test {yaml=ingest_geoip/60_ip_location_databases/Test adding, getting, and removing ip location databases} elastic#138502 ESQL: Refactor HeapAttackIT (elastic#138432) [Inference API] Add ElasticInferenceServiceDenseTextEmbeddingsServiceSettings to InferenceNamedWriteablesProvider (elastic#138484) Store split indices (elastic#138396) ES|QL Update CHUNK to support chunking_settings as optional argument (elastic#138123) Extract common blob-update logic in `S3HttpHandler` (elastic#138490) Cleanup esql request building api (elastic#138398) Round sum and avg in exponential_histogram CSV tests (elastic#138472) ESQL: load exponential_histogram total count as double instead of long (elastic#138417) [SIMD] Use fixed width native types for better Java interoperability (elastic#138429) Do not use Min or Max as Top's surrogate when there is an outputField (elastic#138380) ES|QL: Fix generative tests (elastic#138478) Mute org.elasticsearch.xpack.inference.integration.AuthorizationTaskExecutorIT testCreatesEisChatCompletion_DoesNotRemoveEndpointWhenNoLongerAuthorized elastic#138480 ...
EsqlQueryRequestis structured as a mutable class with builder like setters.There are 2 factory methods to create an instance:
syncEsqlQueryRequestandasyncEsqlQueryRequest.This change makes those to accept a
queryparameter as this is the only mandatory field in the request as well as fixes several setters to be builder like.This is intended to make it obvious that
queryis mandatory and shorten overall query initialization chain.For example
syncEsqlQueryRequest().query("FROM *")becomessyncEsqlQueryRequest("FROM *")