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.

Sign up for GitHub

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

Jump to bottom

SQL: Make Literal a NamedExpression #33583

Merged
merged 3 commits into from
Sep 11, 2018
Merged

SQL: Make Literal a NamedExpression #33583

merged 3 commits into from
Sep 11, 2018

Conversation

costin
Copy link
Member

@costin costin commented Sep 10, 2018

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

Fix #33523

@costin
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 costin requested review from astefan and matriv September 10, 2018 20:24
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

matriv
matriv reviewed Sep 10, 2018
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

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

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java
&& Objects.equals(dataType, other.dataType);
}

@Override
public String toString() {
return Objects.toString(value);
String s = String.valueOf(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Literal.java Outdated
name = foldable instanceof NamedExpression ? ((NamedExpression) foldable).name() : String.valueOf(fold);
}

return new Literal(foldable.location(), name, foldable.fold(), foldable.dataType());
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace foldable.fold() with fold.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java Outdated
@@ -85,6 +85,14 @@

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

private static Literal ONE = L(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: you can also make them final

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java Outdated Show resolved Hide resolved
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());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use the unwrapAlias() here.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java Outdated Show resolved Hide resolved
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java Outdated Show resolved Hide resolved
matriv
matriv reviewed Sep 10, 2018
View reviewed changes
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java Outdated
@@ -13,21 +13,21 @@
import static java.util.Collections.emptyList;

/**
* {@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.
Copy link
Contributor

Choose a reason for hiding this comment

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

clien -> client

matriv
matriv approved these changes Sep 10, 2018
View reviewed changes
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

Left one more comment on the fixup. LGTM otherwise.

astefan
astefan reviewed Sep 11, 2018
View reviewed changes
x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/Attribute.java Outdated
* {@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.
Copy link
Contributor

Choose a reason for hiding this comment

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

"sent to the clienT"

astefan
astefan approved these changes Sep 11, 2018
View reviewed changes
Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM

@costin costin merged commit 91bca17 into elastic:master Sep 11, 2018
@costin costin deleted the fix-33523 branch September 11, 2018 15:47
costin added a commit that referenced this pull request Sep 11, 2018
@costin
SQL: Make Literal a NamedExpression (#33583)
c771d46 
* 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)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 11, 2018
@jasontedor
Merge branch 'master' into upgrade-remote-cluster-settings
7b1e414 
* 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)
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 12, 2018
@jasontedor
Merge branch 'master' into pr/33501
2c3a276 
* 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)
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 12, 2018
@jasontedor
Merge branch 'master' into pr/33367
310ca5d 
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matriv matriv matriv approved these changes

@astefan astefan astefan approved these changes

Assignees
No one assigned
Labels
:Analytics/SQL SQL querying >enhancement v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@costin @elasticmachine @astefan @matriv @colings86