SQL: Make Literal a NamedExpression by costin · Pull Request #33583 · elastic/elasticsearch

costin · 2018-09-10T20:24:51Z

Literal now is a NamedExpression reducing the need for Aliases for
folded expressions leading to simpler optimization rules.

Fix #33523

Literal now is a NamedExpression reducing the need for Aliases for folded expressions leading to simpler optimization rules. Fix elastic#33523

elasticmachine · 2018-09-10T20:24:52Z

Pinging @elastic/es-search-aggs

matriv

Nice to simplify the code! Left some comments and suggestions.

matriv · 2018-09-10T20:46:19Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java

    @Override
    public String toString() {
-        return Objects.toString(value);
+        String s = String.valueOf(value);


Does it make sense to store the String value to a class attribute (minor perf improv if multiple calls on toString())

toString isn't called outside debugging (so few if any calls) and the value -> string is fairly straight.

matriv · 2018-09-10T20:47:43Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java

+            name = foldable instanceof NamedExpression ? ((NamedExpression) foldable).name() : String.valueOf(fold);
+        }
+
+        return new Literal(foldable.location(), name, foldable.fold(), foldable.dataType());


You can replace foldable.fold() with fold.

matriv · 2018-09-10T20:52:02Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java


    private static final Expression DUMMY_EXPRESSION = new DummyBooleanExpression(EMPTY, 0);

+    private static Literal ONE = L(1);


Minor: you can also make them final

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

matriv · 2018-09-10T20:56:05Z

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java

-        c = ((Alias) result).child();
-        assertTrue(c instanceof Literal);
-        assertEquals(5, ((Literal) c).value());
+        assertEquals(5, ((Literal) result).value());


I think you can use the unwrapAlias() here.

I've removed the method since it provide much value (if any) and was used in only one place.

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java

matriv · 2018-09-10T21:21:47Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java

 /**
- * {@link Expression}s that can be materlized to the user, representing the result
- * columns. Typically are converted into constants, functions or Elasticsearch order-bys,
+ * {@link Expression}s that can be materialized and represent the result columns sent to the clien.


clien -> client

matriv

Left one more comment on the fixup. LGTM otherwise.

astefan · 2018-09-11T06:29:17Z

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java

- * {@link Expression}s that can be converted into Elasticsearch
- * sorts, aggregations, or queries. They can also be extracted
- * from the result of a search.
+ * {@link Expression}s that can be materialized and represent the result columns sent to the clien.


"sent to the clienT"

astefan

LGTM

* SQL: Make Literal a NamedExpression Literal now is a NamedExpression reducing the need for Aliases for folded expressions leading to simpler optimization rules. Fix #33523 (cherry picked from commit 91bca17)

* master: Remove debug logging in full cluster restart tests (elastic#33612) Expose CCR to the transport client (elastic#33608) Mute testIndexDeletionWhenNodeRejoins SQL: Make Literal a NamedExpression (elastic#33583) [DOCS] Adds missing built-in user information (elastic#33585) Improves doc values format deprecation message (elastic#33576)

* master: (43 commits) [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603) Update AWS SDK to 1.11.406 in repository-s3 (elastic#30723) Expose CCR stats to monitoring (elastic#33617) [Docs] Update match-query.asciidoc (elastic#33610) TEST: Adjust rollback condition when shard is empty [CCR] Improve shard follow task's retryable error handling (elastic#33371) Forbid negative `weight` in Function Score Query (elastic#33390) Clarify context suggestions filtering and boosting (elastic#33601) Disable CCR REST endpoints if CCR disabled (elastic#33619) Lower version on full cluster restart settings test Upgrade remote cluster settings (elastic#33537) NETWORKING: http.publish_host Should Contain CNAME (elastic#32806) Add test coverage for global checkpoint listeners Reset replica engine to global checkpoint on promotion (elastic#33473) HLRC: ML Delete Forecast API (elastic#33526) Remove debug logging in full cluster restart tests (elastic#33612) Expose CCR to the transport client (elastic#33608) Mute testIndexDeletionWhenNodeRejoins SQL: Make Literal a NamedExpression (elastic#33583) [DOCS] Adds missing built-in user information (elastic#33585) ...

* master: (128 commits) [HLRC][ML] Add ML put datafeed API to HLRC (elastic#33603) Update AWS SDK to 1.11.406 in repository-s3 (elastic#30723) Expose CCR stats to monitoring (elastic#33617) [Docs] Update match-query.asciidoc (elastic#33610) TEST: Adjust rollback condition when shard is empty [CCR] Improve shard follow task's retryable error handling (elastic#33371) Forbid negative `weight` in Function Score Query (elastic#33390) Clarify context suggestions filtering and boosting (elastic#33601) Disable CCR REST endpoints if CCR disabled (elastic#33619) Lower version on full cluster restart settings test Upgrade remote cluster settings (elastic#33537) NETWORKING: http.publish_host Should Contain CNAME (elastic#32806) Add test coverage for global checkpoint listeners Reset replica engine to global checkpoint on promotion (elastic#33473) HLRC: ML Delete Forecast API (elastic#33526) Remove debug logging in full cluster restart tests (elastic#33612) Expose CCR to the transport client (elastic#33608) Mute testIndexDeletionWhenNodeRejoins SQL: Make Literal a NamedExpression (elastic#33583) [DOCS] Adds missing built-in user information (elastic#33585) ...

SQL: Make Literal a NamedExpression

29300af

Literal now is a NamedExpression reducing the need for Aliases for folded expressions leading to simpler optimization rules. Fix elastic#33523

costin added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.5.0 labels Sep 10, 2018

costin requested review from astefan and matriv September 10, 2018 20:24

matriv reviewed Sep 10, 2018

View reviewed changes

Address feedback

37610d3

matriv reviewed Sep 10, 2018

View reviewed changes

matriv approved these changes Sep 10, 2018

View reviewed changes

astefan reviewed Sep 11, 2018

View reviewed changes

astefan approved these changes Sep 11, 2018

View reviewed changes

Fix typo.

48ebc6a

costin merged commit 91bca17 into elastic:master Sep 11, 2018

costin deleted the fix-33523 branch September 11, 2018 15:47

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019


		private static final Expression DUMMY_EXPRESSION = new DummyBooleanExpression(EMPTY, 0);

		private static Literal ONE = L(1);

Conversation

costin commented Sep 10, 2018

Uh oh!

elasticmachine commented Sep 10, 2018

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants