Skip to content

[ESQL] Clean up UNSUPPORTED type blocks#111648

Merged
elasticsearchmachine merged 2 commits intoelastic:mainfrom
not-napoleon:esql-tests-for-unsupported-blocks
Aug 6, 2024
Merged

[ESQL] Clean up UNSUPPORTED type blocks#111648
elasticsearchmachine merged 2 commits intoelastic:mainfrom
not-napoleon:esql-tests-for-unsupported-blocks

Conversation

@not-napoleon
Copy link
Copy Markdown
Member

I hit an NPE while testing against creating unsupported blocks. We were trying to create a bytes ref of null, which isn't valid. I've changed the test code to instead use append null. While investigating that, I found UnsupportedValuesSource, which seems vestigial. The only uses I found for it were references to the UNSUPPORTED_OUTPUT constant, which is just null, one of which was the erroneous BytesRef constructor I was trying to fix. I think in this case the named constant actually decreased the readability, as it was not obvious that we were passing in a null there (although Intellij did flag it with a warning). At any rate, I've inlined the two other uses of that constant and removed the otherwise-unused class.

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 6, 2024
@not-napoleon not-napoleon added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 6, 2024
@elasticsearchmachine elasticsearchmachine merged commit a78a21b into elastic:main Aug 6, 2024
@not-napoleon not-napoleon deleted the esql-tests-for-unsupported-blocks branch August 6, 2024 18:55
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Aug 7, 2024
* upstream/main: (132 commits)
  Fix compile after several merges
  Update docs with new behavior on skip conditions (elastic#111640)
  Skip on any instance of node or version features being present (elastic#111268)
  Skip on any node capability being present (elastic#111585)
  [DOCS] Publishes Anthropic inference service docs. (elastic#111619)
  Introduce `ChunkedZipResponse` (elastic#109820)
  [Gradle] fix esql compile cacheability (elastic#111651)
  Mute org.elasticsearch.datastreams.logsdb.qa.StandardVersusLogsIndexModeChallengeRestIT testTermsQuery elastic#111666
  Mute org.elasticsearch.datastreams.logsdb.qa.StandardVersusLogsIndexModeChallengeRestIT testMatchAllQuery elastic#111664
  Mute org.elasticsearch.xpack.esql.analysis.VerifierTests testMatchCommand elastic#111661
  Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommandWithMultipleMatches {default} elastic#111660
  Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommand {default} elastic#111659
  Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommandWithWhereClause {default} elastic#111658
  LogsDB qa tests - add specific matcher for source (elastic#111568)
  ESQL: Move `randomLiteral` (elastic#111647)
  [ESQL] Clean up UNSUPPORTED type blocks (elastic#111648)
  ESQL: Remove the `NESTED` DataType (elastic#111495)
  ESQL: Move more out of esql-core (elastic#111604)
  Improve MvPSeriesWeightedSum edge case and add more tests (elastic#111552)
  Add link to flood-stage watermark exception message (elastic#111315)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/TransportVersions.java
rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Aug 7, 2024
I hit an NPE while testing against creating unsupported blocks.  We were
trying to create a bytes ref of `null`, which isn't valid. I've changed
the test code to instead use append null.  While investigating that, I
found `UnsupportedValuesSource`, which seems vestigial.  The only uses I
found for it were references to the `UNSUPPORTED_OUTPUT` constant, which
is just `null`, one of which was the erroneous `BytesRef` constructor I
was trying to fix.  I think in this case the named constant actually
decreased the readability, as it was not obvious that we were passing in
a null there (although Intellij did flag it with a warning).  At any
rate, I've inlined the two other uses of that constant and removed the
otherwise-unused class.
mhl-b pushed a commit that referenced this pull request Aug 8, 2024
I hit an NPE while testing against creating unsupported blocks.  We were
trying to create a bytes ref of `null`, which isn't valid. I've changed
the test code to instead use append null.  While investigating that, I
found `UnsupportedValuesSource`, which seems vestigial.  The only uses I
found for it were references to the `UNSUPPORTED_OUTPUT` constant, which
is just `null`, one of which was the erroneous `BytesRef` constructor I
was trying to fix.  I think in this case the named constant actually
decreased the readability, as it was not obvious that we were passing in
a null there (although Intellij did flag it with a warning).  At any
rate, I've inlined the two other uses of that constant and removed the
otherwise-unused class.
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Sep 4, 2024
I hit an NPE while testing against creating unsupported blocks.  We were
trying to create a bytes ref of `null`, which isn't valid. I've changed
the test code to instead use append null.  While investigating that, I
found `UnsupportedValuesSource`, which seems vestigial.  The only uses I
found for it were references to the `UNSUPPORTED_OUTPUT` constant, which
is just `null`, one of which was the erroneous `BytesRef` constructor I
was trying to fix.  I think in this case the named constant actually
decreased the readability, as it was not obvious that we were passing in
a null there (although Intellij did flag it with a warning).  At any
rate, I've inlined the two other uses of that constant and removed the
otherwise-unused class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants