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.

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

Fix compiling error by adding meta fields checker before 7DOT8 in legacy SQL #2701

Conversation

LantaoJin
Copy link
Member

@LantaoJin LantaoJin commented May 31, 2024

Description

Fix compiling error by adding meta fields checker before 7DOT8 in legacy SQL

Issues Resolved

#2699

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@LantaoJin
Copy link
Member Author

LantaoJin commented May 31, 2024

Could any maintainer help to rerun failed CI?

@ykmr1224
Copy link
Collaborator

The original comment for the deleted constant mentions that isMetadataField should be used instead. Did you seek the possibility to use that? I think the background is that other plugin could define metadata field, and usage of constant is not the best way.

@@ -41,6 +43,19 @@ public class Util {

public static final String NESTED_JOIN_TYPE = "NestedJoinType";

public static final Set<String> META_FIELDS_BEFORE_7DOT8 =
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Can we wrap this with a static method so we can change implementation easily?
  2. If we go ahead with this approach, please add comment regarding this is not ideal solution.
  3. What does the suffix 7D0T8 mean?

Copy link
Member

@cwperks cwperks May 31, 2024

Choose a reason for hiding this comment

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

7DOT8 is referring to ES 7.8.

"_ttl" and "_timestamp" were removed in ES6. See https://www.elastic.co/blog/elasticsearch-5-0-0-alpha4-released

"_type" was renamed to "_nested_path" in OS 2: opensearch-project/OpenSearch#3196

"_size" is not a built-in metadata mapper, it comes from the mapper-size plugin which is not installed by default. Using mapperService.isMetadataField("_size") should return true if the mapper-size plugin is installed in the cluster.

The other values in this list still exist and are built-in metadata fields so mapperService.isMetadataField() should return true.

@@ -41,6 +43,19 @@ public class Util {

public static final String NESTED_JOIN_TYPE = "NestedJoinType";

public static final Set<String> META_FIELDS_BEFORE_7DOT8 =
Collections.unmodifiableSet(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It's simpler to use ImmutableSet.of

@ykmr1224
Copy link
Collaborator

Revert commit PR is raised and we can be unblocked once that is merged.

@LantaoJin
Copy link
Member Author

Yep, revert PR merged, close this one.

@LantaoJin LantaoJin closed this Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants