SQL: Deprecate index_include_frozen request parameter and `FROZEN` keyword #83943

Luegg · 2022-02-15T13:49:39Z

Resolves #81939 (part of #70192)

Deprecates the index_include_frozen REST parameter in the /_sql endpoint and all the syntax including the FROZEN keyword.

elasticmachine · 2022-02-15T13:49:42Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2022-02-15T13:50:05Z

Hi @Luegg, I've created a changelog YAML for you. Note that since this PR is labelled >deprecation, you need to update the changelog YAML to fill out the extended information sections.

Luegg · 2022-02-16T07:42:27Z

@elasticmachine run elasticsearch-ci/part-2

elasticsearchmachine · 2022-02-16T08:27:04Z

Hi @Luegg, I've updated the changelog YAML for you. Note that since this PR is labelled >deprecation, you need to update the changelog YAML to fill out the extended information sections.

docs/changelog/83943.yaml

costin

Could you please add a test (to assert that the warning doesn't reach the driver)?
There needs to be a follow-up to plug the warnings to both JDBC and ODBC since said mechanism is not in place (and likely that might affect the communication protocol).

LGTM.

costin · 2022-02-17T13:23:31Z

Worth checking and doing a similar PR for EQL.

Luegg · 2022-02-21T08:41:16Z

Could you please add a test (to assert that the warning doesn't reach the driver)?

Do you mean to assert that the Warning header is not set for request using JDBC/ODBC/CLI mode? Or just asserting it's not passed through as a warning to the JDBC interface?

Luegg · 2022-02-21T08:41:44Z

Worth checking and doing a similar PR for EQL.

As far as I can tell, EQL does not support frozen indices.

Luegg · 2022-02-23T09:23:36Z

@elasticmachine run elasticsearch-ci/part-2

astefan

Looks good. Left some comments.

astefan · 2022-02-23T09:50:52Z

...ver/single-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/JdbcWarningsIT.java

+import java.sql.ResultSet;
+import java.sql.Statement;
+
+public class JdbcWarningsIT extends JdbcIntegrationTestCase {


Any reason for having this test only in single_node scenario? With regards to multi_node it's probably not worth considering it, but how about the security tests suite?

👍 I also saw that this test is better put to the JDBC test cases in org.elasticsearch.xpack.sql.qa.jdbc instead of org.elasticsearch.xpack.sql.qa

astefan · 2022-02-23T09:52:54Z

...ngle-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/RestSqlDeprecationIT.java

+        testFrozenSyntaxIsDeprecated("SELECT * FROM FROZEN test", "FROZEN");
+    }
+
+    public void testFrozenSyntaxIsDeprecated(String query, String syntax) throws IOException {


astefan · 2022-02-23T09:52:58Z

...ngle-node/src/test/java/org/elasticsearch/xpack/sql/qa/single_node/RestSqlDeprecationIT.java

+        );
+    }
+
+    public void testDeprecationWarning(RequestObjectBuilder query, String warning) throws IOException {


astefan · 2022-02-23T09:59:20Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

+    private static final String FROZEN_DEPRECATION_WARNING = "[{}] syntax is deprecated because frozen indices have been deprecated. "
+        + "Consider cold or frozen tiers in place of frozen indices.";
+
+    protected void warnDeprecatedFrozenSyntax(boolean includeFrozen, String syntax) {


A better name for this method is maybeWarnDeprecatedFrozenSyntax since the logic on issuing the message depends on the value of the parameter. Or, keep the same name, but move the if (includeFrozen) decision to the calling code.

astefan

LGTM. Thank you

elasticsearchmachine · 2022-02-23T13:53:18Z

💔 Backport failed

The backport operation could not be completed due to the following error:
An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 83943

…yword (elastic#83943) Resolves elastic#81939 (part of elastic#70192) Deprecates the `index_include_frozen` REST parameter in the `/_sql` endpoint and all the syntax including the `FROZEN` keyword. # Conflicts: # x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

…yword (#83943) (#84298) Resolves #81939 (part of #70192) Deprecates the `index_include_frozen` REST parameter in the `/_sql` endpoint and all the syntax including the `FROZEN` keyword. # Conflicts: # x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

…yword (elastic#83943) Resolves elastic#81939 (part of elastic#70192) Deprecates the `index_include_frozen` REST parameter in the `/_sql` endpoint and all the syntax including the `FROZEN` keyword.

Follow up on #83943 to ensure that users of the JDBC drivers also can act on the warnings. The change ensures that warnings in the response headers from the `_sql` API are available in `JdbcResultSet.getWarnings()`.

Follow up on elastic#83943 to ensure that users of the JDBC drivers also can act on the warnings. The change ensures that warnings in the response headers from the `_sql` API are available in `JdbcResultSet.getWarnings()`.

* SQL: forward warning headers to JDBC driver (#84499) Follow up on #83943 to ensure that users of the JDBC drivers also can act on the warnings. The change ensures that warnings in the response headers from the `_sql` API are available in `JdbcResultSet.getWarnings()`. * fix version

SQL: Deprecate index_include_frozen parameter

1b2ebd9

Luegg added >deprecation :Analytics/SQL SQL querying v7.17.1 v8.2.0 v8.1.1 labels Feb 15, 2022

Luegg requested review from costin, luigidellaquila, astefan and bpintea February 15, 2022 13:49

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Feb 15, 2022

Update docs/changelog/83943.yaml

2f50d4e

Luegg marked this pull request as draft February 15, 2022 13:53

Luegg removed request for costin, luigidellaquila, astefan and bpintea February 15, 2022 13:54

Fix build failures

f74839f

Luegg force-pushed the deprecate/frozen branch from e5e8ef5 to f74839f Compare February 15, 2022 17:04

Merge origin/main into deprecate/frozen

854cefd

Luegg removed the v7.17.1 label Feb 16, 2022

Luegg changed the title ~~SQL: Deprecate index_include_frozen request parameter~~ SQL: Deprecate index_include_frozen request parameter and FROZEN keyword Feb 16, 2022

deprecate FROZEN keyword as well

a19741a

Luegg force-pushed the deprecate/frozen branch from 30aad46 to a19741a Compare February 16, 2022 08:45

update changelog yml

5afb9f6

pugnascotia reviewed Feb 16, 2022

View reviewed changes

docs/changelog/83943.yaml Outdated Show resolved Hide resolved

costin approved these changes Feb 17, 2022

View reviewed changes

Luegg added 3 commits February 21, 2022 10:56

Merge origin/main into deprecate/frozen

b743efc

add test for warnings in JDBC driver

f2e567e

Merge origin/main into deprecate/frozen

7d5a577

astefan reviewed Feb 23, 2022

View reviewed changes

Luegg requested a review from astefan February 23, 2022 12:27

address comments

7f946e2

Luegg force-pushed the deprecate/frozen branch from 285012e to 7f946e2 Compare February 23, 2022 12:58

astefan approved these changes Feb 23, 2022

View reviewed changes

Luegg added auto-backport-and-merge Automatically create backport pull requests and merge when ready auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Feb 23, 2022

elasticsearchmachine merged commit 7900a0b into elastic:master Feb 23, 2022

Luegg deleted the deprecate/frozen branch February 23, 2022 13:43

Luegg mentioned this pull request Feb 23, 2022

[8.1] SQL: Deprecate index_include_frozen request parameter and FROZEN keyword (#83943) #84298

Merged

Luegg mentioned this pull request Mar 1, 2022

SQL: forward warning headers to JDBC driver #84499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: Deprecate index_include_frozen request parameter and `FROZEN` keyword #83943

SQL: Deprecate index_include_frozen request parameter and `FROZEN` keyword #83943

Luegg commented Feb 15, 2022 •

edited

Loading

elasticmachine commented Feb 15, 2022

elasticsearchmachine commented Feb 15, 2022

Luegg commented Feb 16, 2022

elasticsearchmachine commented Feb 16, 2022

costin left a comment

costin commented Feb 17, 2022

Luegg commented Feb 21, 2022

Luegg commented Feb 21, 2022

Luegg commented Feb 23, 2022

astefan left a comment

astefan Feb 23, 2022

Luegg Feb 23, 2022

astefan Feb 23, 2022

astefan Feb 23, 2022

astefan Feb 23, 2022

astefan left a comment

elasticsearchmachine commented Feb 23, 2022

SQL: Deprecate index_include_frozen request parameter and FROZEN keyword #83943

SQL: Deprecate index_include_frozen request parameter and FROZEN keyword #83943

Conversation

Luegg commented Feb 15, 2022 • edited Loading

elasticmachine commented Feb 15, 2022

elasticsearchmachine commented Feb 15, 2022

Luegg commented Feb 16, 2022

elasticsearchmachine commented Feb 16, 2022

costin left a comment

Choose a reason for hiding this comment

costin commented Feb 17, 2022

Luegg commented Feb 21, 2022

Luegg commented Feb 21, 2022

Luegg commented Feb 23, 2022

astefan left a comment

Choose a reason for hiding this comment

astefan Feb 23, 2022

Choose a reason for hiding this comment

Luegg Feb 23, 2022

Choose a reason for hiding this comment

astefan Feb 23, 2022

Choose a reason for hiding this comment

astefan Feb 23, 2022

Choose a reason for hiding this comment

astefan Feb 23, 2022

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Feb 23, 2022

💔 Backport failed

SQL: Deprecate index_include_frozen request parameter and `FROZEN` keyword #83943

SQL: Deprecate index_include_frozen request parameter and `FROZEN` keyword #83943

Luegg commented Feb 15, 2022 •

edited

Loading