[ES|QL] Remove EsqlDataTypes#111089
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
nik9000
left a comment
There was a problem hiding this comment.
Awesome. @not-napoleon will probably want to see it too.
luigidellaquila
left a comment
There was a problem hiding this comment.
Awesome, thank you very much @ioanatia!
I left only one comment about a method name that doesn't really match its logic.
x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/type/DataType.java
Outdated
Show resolved
Hide resolved
astefan
left a comment
There was a problem hiding this comment.
I've looked especially at the change in the Analyzer regarding isPrimitive method call and when we removed UNSUPPORTED from the isPrimitive method and why.
Without testing if everything is green again, we should keep isPrimitive with the original implementation (no UNSUPPORTED data type there: return t != OBJECT && t != NESTED).
The "other" isPrimitive method (which is sad it exists) should be renamed to isPrimitiveAndSupported and it should have the same implementation as of now (meaning return isPrimitive(t) && t != UNSUPPORTED). They should both live in DataType class.
Thanks.
not-napoleon
left a comment
There was a problem hiding this comment.
LGTM. I've been meaning to do this refactor for a while, thank you for getting to it!
|
@astefan I don't think we need both Which occurs in Given that, I think we can adopt the More generally, I am concerned that these functions using a |
Thank you all for the feedback! |
bpintea
left a comment
There was a problem hiding this comment.
I also think this is the right way to merge the classes.
Like, is it correct that isPrimitive(PARTIAL_AGG) == true?
Probably not, but I'd think this would be a isPrimitive() concern. There is a need to exclude UNSUPPORTED privitives.
Otoh, I think I'd have called the function isSupportedPrimitive() (since it's not isPrimitiveAndNotUnsupported()), but that's a nit, AFAI'm concerned it can be merged as is.
* main: (39 commits) Update README.asciidoc (elastic#111244) ESQL: INLINESTATS (elastic#109583) ESQL: Document a little of `DataType` (elastic#111250) Relax assertions in segment level field stats (elastic#111243) LogsDB data generator - support nested object field (elastic#111206) Validate `Authorization` header in Azure test fixture (elastic#111242) Fixing HistoryStoreTests.testPut() and testStoreWithHideSecrets() (elastic#111246) [ESQL] Remove Named Expcted Types map from testing infrastructure (elastic#111213) Change visibility of createWriter to allow tests from a different package to override it (elastic#111234) [ES|QL] Remove EsqlDataTypes (elastic#111089) Mute org.elasticsearch.repositories.azure.AzureBlobContainerRetriesTests testReadNonexistentBlobThrowsNoSuchFileException elastic#111233 Abstract codec lookup by name, to make CodecService extensible (elastic#111007) Add HTTPS support to `AzureHttpFixture` (elastic#111228) Unmuting tests related to free_context action being processed in ESSingleNodeTestCase (elastic#111224) Upgrade Azure SDK (elastic#111225) Collapse transport versions for 8.14.0 (elastic#111199) Make sure contender uses logs templates (elastic#111183) unmute HistogramPercentileAggregationTests.testBoxplotHistogram (elastic#111223) Refactor Quality Assurance test infrastructure (elastic#111195) Mute org.elasticsearch.xpack.restart.FullClusterRestartIT testDisableFieldNameField {cluster=UPGRADED} elastic#111222 ... # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
follows up on #111016 (comment) and moves
areCompatibletoDataType.furthermore it completely removes
EsqlDataTypeswhich I hope won't be too disruptive 🙈