[GitHub] [calcite] vlsi commented on a change in pull request #1794: [CALCITE-3781] HintStrategy can specify excluded rules for planner
vlsi commented on a change in pull request #1794: [CALCITE-3781] HintStrategy
can specify excluded rules for planner
URL: https://github.com/apache/calcite/pull/1794#discussion_r378081870
##
File path:
core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
##
@@ -160,6 +160,11 @@ protected void onMatch() {
return;
}
+ if (isRuleExcluded()) {
+LOGGER.debug("Rule [{}] not fired due to exclusion hint", getRule());
Review comment:
Should this be evaluated before the rule is added to the match queue?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1790: [CALCITE-3769] Deprecate TableScanRule
vlsi commented on a change in pull request #1790: [CALCITE-3769] Deprecate
TableScanRule
URL: https://github.com/apache/calcite/pull/1790#discussion_r378080137
##
File path: core/src/main/java/org/apache/calcite/tools/RelRunners.java
##
@@ -29,6 +35,19 @@ private RelRunners() {}
/** Runs a relational expression by creating a JDBC connection. */
public static PreparedStatement run(RelNode rel) {
+final RelShuttle shuttle = new RelHomogeneousShuttle() {
+ @Override public RelNode visit(TableScan scan) {
+final RelOptTable table = scan.getTable();
+if (scan instanceof LogicalTableScan
+&& Bindables.BindableTableScan.canHandle(table)) {
+ // Always replace the LogicalTableScan with BindableTableScan
+ // because it's implementation does not require a "schema" as
context.
+ return Bindables.BindableTableScan.create(scan.getCluster(), table);
Review comment:
This looks odd.
Can you clarify why is this replacement needed?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
vlsi commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter URL: https://github.com/apache/calcite/pull/1792#issuecomment-585068753 > It is uncertain whether a Schema's concrete implementation (e.g., JdbcSchema) can push down case insensitive lookups to underlying datasources Calcite should not pretend it can do case insensitive search if the only downstream API is case sensitive. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] DonnyZone commented on issue #1798: [CALCITE-2442] Remove .toDelete cassandra temp folder on Windows after tests
DonnyZone commented on issue #1798: [CALCITE-2442] Remove .toDelete cassandra temp folder on Windows after tests URL: https://github.com/apache/calcite/pull/1798#issuecomment-585044086 This fix still fails to delete the `.toDelete` file. Are there any approach to figure out which thread lock the file? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 edited a comment on issue #1776: [CALCITE-3724] Presto dialect implementation
danny0405 edited a comment on issue #1776: [CALCITE-3724] Presto dialect implementation URL: https://github.com/apache/calcite/pull/1776#issuecomment-585018445 > > Thanks for the PR, can you explain why we need a Presto dialect ? Which unparse logic do you want to customize ? > > hi @danny0405 The previous discussion is here [1] > [1] https://mail-archives.apache.org/mod_mbox/calcite-dev/202001.mbox/%3cCAPSgeETHa88DN7Bwqd+qez-i1=3xda2ln3+wqdk7x_qfexm...@mail.gmail.com%3e Thanks, you can paste the conclusion/the background to the JIRA issue or the link is okey. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1776: [CALCITE-3724] Presto dialect implementation
danny0405 commented on issue #1776: [CALCITE-3724] Presto dialect implementation URL: https://github.com/apache/calcite/pull/1776#issuecomment-585018445 > > Thanks for the PR, can you explain why we need a Presto dialect ? Which unparse logic do you want to customize ? > > hi @danny0405 The previous discussion is here [1] > [1] https://mail-archives.apache.org/mod_mbox/calcite-dev/202001.mbox/%3cCAPSgeETHa88DN7Bwqd+qez-i1=3xda2ln3+wqdk7x_qfexm...@mail.gmail.com%3e Thanks, you can past the conclusion/the background to the JIRA issue or the link is okey. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] chunweilei commented on a change in pull request #1799: [CALCITE-3785] HepPlanner.belongToDag() doesn't have to use mapDigest…
chunweilei commented on a change in pull request #1799: [CALCITE-3785]
HepPlanner.belongToDag() doesn't have to use mapDigest…
URL: https://github.com/apache/calcite/pull/1799#discussion_r378005146
##
File path: core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java
##
@@ -500,8 +500,7 @@ private void applyRules(
/** Returns whether the vertex is valid. */
private boolean belongsToDag(HepRelVertex vertex) {
-Pair> key = key(vertex.getCurrentRel());
-return mapDigestToVertex.get(key) != null;
+return graph.vertexSet().contains(vertex);
Review comment:
Agree with hsyuan.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 merged pull request #1794: [CALCITE-3781] HintStrategy can specify excluded rules for planner
danny0405 merged pull request #1794: [CALCITE-3781] HintStrategy can specify excluded rules for planner URL: https://github.com/apache/calcite/pull/1794 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[calcite] branch master updated (15fe434 -> eaa40cb)
This is an automated email from the ASF dual-hosted git repository. danny0405 pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from 15fe434 Site: Update IntelliJ instructions with suggested and problematic versions add eaa40cb [CALCITE-3781] HintStrategy can specify excluded rules for planner No new revisions were added by this update. Summary of changes: .../apache/calcite/plan/AbstractRelOptPlanner.java | 6 + .../org/apache/calcite/plan/RelOptRuleCall.java| 16 ++ .../calcite/plan/volcano/VolcanoRuleCall.java | 5 + .../apache/calcite/rel/hint/HintStrategies.java| 2 +- .../apache/calcite/rel/hint/HintStrategyTable.java | 193 + .../apache/calcite/test/SqlHintsConverterTest.java | 80 ++--- .../apache/calcite/test/SqlHintsConverterTest.xml | 15 ++ 7 files changed, 262 insertions(+), 55 deletions(-)
[GitHub] [calcite] DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup
methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#issuecomment-584975246
(1) `Schema` interface is provided to users. We'd better not bring much
burden to users, even a `default` implementation. It is uncertain whether a
`Schema`'s concrete implementation (e.g., `JdbcSchema`) can push down case
insensitive lookups to underlying datasources. If not, we still need to obtain
all tables.
Insteadly, it is more reasonable that Calcite implements case insensitive
lookup in `CalciteSchema`. From my understanding (correct me if I am wrong),
the initial design might be:
```
Schema: interface for users (case sensitive)
|
CalciteSchema: wrapper around user-defined schema used internally, providing
case (in)sensitive lookups
|
CachingCalciteSchema/SimpleCalciteSchema
```
(2)
> Most of the implementations should be able to delegate case insensitive
requests to NameMap, which would be OK for performance.
I think `CachingCalciteSchema` has already achieved this goal. It caches
tables and maintains them in `NameMap`.
The javadoc of `CachingCalciteSchema` is:
> * Concrete implementation of {@link CalciteSchema} that caches tables,
> * functions and sub-schemas.
While the javadoc of `SimpleCalciteSchema` is
> * A concrete implementation of {@link
org.apache.calcite.jdbc.CalciteSchema}
> * that maintains minimal state.
`SimpleCalciteSchema` and `CachingCalciteSchema` play different roles. If we
unify them for the seek of performance, we can just remove
`SimpleCalciteSchema`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup
methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#issuecomment-584975246
(1) `Schema` interface is provided to users. We'd better not bring much
burden to users, even a `default` implementation. Moreover, it is uncertain
whether a `Schema`'s concrete implementation (e.g., `JdbcSchema`) can push down
case insensitive lookups to underlying datasources.
Insteadly, it is more reasonable that Calcite implements case insensitive
lookup in `CalciteSchema`. From my understanding (correct me if I am wrong),
the initial design might be:
```
Schema: interface for users (case sensitive)
|
CalciteSchema: wrapper around user-defined schema used internally, providing
case (in)sensitive lookups
|
CachingCalciteSchema/SimpleCalciteSchema
```
(2)
> Most of the implementations should be able to delegate case insensitive
requests to NameMap, which would be OK for performance.
I think `CachingCalciteSchema` has already achieved this goal. It caches
tables and maintains them in `NameMap`.
The javadoc of `CachingCalciteSchema` is:
> * Concrete implementation of {@link CalciteSchema} that caches tables,
> * functions and sub-schemas.
While the javadoc of `SimpleCalciteSchema` is
> * A concrete implementation of {@link
org.apache.calcite.jdbc.CalciteSchema}
> * that maintains minimal state.
`SimpleCalciteSchema` and `CachingCalciteSchema` play different roles. If we
unify them for the seek of performance, we can just remove
`SimpleCalciteSchema`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup
methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#issuecomment-584975246
(1) `Schema` interface is provided to users. We'd better not bring much
burden to users, even a `default` implementation. Moreover, it is uncertain
whether a `Schema`'s concrete implementation (e.g., `JdbcSchema`) can push down
case insensitive lookups to underlying datasources.
Insteadly, `CalciteSchema` is a more suitable place to implement case
insensitive lookup. From my understanding (correct me if I am wrong), the
initial design may be:
```
Schema: interface for users (case sensitive)
|
CalciteSchema: wrapper around user-defined schema used internally, providing
case (in)sensitive lookups
|
CachingCalciteSchema/SimpleCalciteSchema
```
(2)
> Most of the implementations should be able to delegate case insensitive
requests to NameMap, which would be OK for performance.
I think `CachingCalciteSchema` has already achieved this goal. It caches
tables and maintains them in `NameMap`.
The javadoc of `CachingCalciteSchema` is:
> * Concrete implementation of {@link CalciteSchema} that caches tables,
> * functions and sub-schemas.
While the javadoc of `SimpleCalciteSchema` is
> * A concrete implementation of {@link
org.apache.calcite.jdbc.CalciteSchema}
> * that maintains minimal state.
`SimpleCalciteSchema` and `CachingCalciteSchema` play different roles. If we
unify them for the seek of performance, we can just remove
`SimpleCalciteSchema`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup
methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#issuecomment-584975246
(1) `Schema` interface is provided to users. We'd better not bring much
burden to users, even a `default` implementation. Moreover, it is uncertain
whether a `Schema`'s concrete implementation (e.g., `JdbcSchema`) can push down
case insensitive lookups to underlying datasources.
Insteadly, `CalciteSchema` is a suitable place to implement case insensitive
lookup. From my understanding (correct me if I am wrong), the initial design
may be:
```
Schema: interface for users (case sensitive)
|
CalciteSchema: wrapper around user-defined schema used internally, providing
case (in)sensitive lookups
|
CachingCalciteSchema/SimpleCalciteSchema
```
(2)
> Most of the implementations should be able to delegate case insensitive
requests to NameMap, which would be OK for performance.
I think `CachingCalciteSchema` has already achieved this goal. It caches
tables and maintains them in `NameMap`.
The javadoc of `CachingCalciteSchema` is:
> * Concrete implementation of {@link CalciteSchema} that caches tables,
> * functions and sub-schemas.
While the javadoc of `SimpleCalciteSchema` is
> * A concrete implementation of {@link
org.apache.calcite.jdbc.CalciteSchema}
> * that maintains minimal state.
`SimpleCalciteSchema` and `CachingCalciteSchema` play different roles. If we
unify them for the seek of performance, we can just remove
`SimpleCalciteSchema`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on issue #1792: [CALCITE-3775] Implicit lookup methods in
SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#issuecomment-584975246
(1) `Schema` interface is provided to users. We'd better not bring much
burden to users, even a `default` implementation. Moreover, it is uncertain
whether a `Schema`'s concrete implementation (e.g., `JdbcSchema`) can push down
case insensitive lookups to underlying datasources.
Insteadly, `CalciteSchema` is a suitable place to implement case insensitive
lookup. From my understanding (correct me if I am wrong), the initial design
may be:
```
Schema: interface for users (case sensitive)
|
CalciteSchema: wrapper around user-defined schema used internally, providing
case (in)sensitive lookups
|
CachingCalciteSchema/SimpleCalciteSchema
```
(2)
> Most of the implementations should be able to delegate case insensitive
requests to NameMap, which would be OK for performance.
I think `CachingCalciteSchema` has already achieved this goal. It caches
tables and maintains them in `NameMap`.
The javadoc of `CachingCalciteSchema` is:
> * Concrete implementation of {@link CalciteSchema} that caches tables,
> * functions and sub-schemas.
While the javadoc of `SimpleCalciteSchema` is
> * A concrete implementation of {@link
org.apache.calcite.jdbc.CalciteSchema}
> * that maintains minimal state.
Therefore, `SimpleCalciteSchema` and `CachingCalciteSchema` play different
roles. If we unify them for the seek of performance, we can just remove
`SimpleCalciteSchema`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone edited a comment on issue #1792: [CALCITE-3775] Implicit lookup
methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#issuecomment-584975246
(1) `Schema` interface is provided to users. We'd better not bring much
burden to users, even a `default` implementation. Moreover, it is uncertain
whether a `Schema`'s concrete implementation (e.g., `JdbcSchema`) can push down
case insensitive lookups to underlying datasources.
Insteadly, `CalciteSchema` is a suitable place to implement case insensitive
lookup. From my understanding (correct me if I am wrong), the initial design
may be:
```
Schema: interface for users (case sensitive)
|
CalciteSchema: wrapper around user-defined schema used internally, providing
case (in)sensitive lookups
|
CachingCalciteSchema/SimpleCalciteSchema
```
(2)
> Most of the implementations should be able to delegate case insensitive
requests to NameMap, which would be OK for performance.
I think `CachingCalciteSchema` has already achieved this goal. It caches
tables and maintains them in `NameMap`.
The javadoc of `CachingCalciteSchema` is:
> * Concrete implementation of {@link CalciteSchema} that caches tables,
> * functions and sub-schemas.
While the javadoc of `SimpleCalciteSchema` is
> * A concrete implementation of {@link
org.apache.calcite.jdbc.CalciteSchema}
> * that maintains minimal state.
Therefore, `SimpleCalciteSchema` and `CachingCalciteSchema` play different
roles. If we unify them for the seek of performance, we can just remove
`SimpleCalciteSchema`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] julianhyde commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
julianhyde commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584933425 Can we move the discussion to JIRA? I never look at closed pull requests, so we're shouting into the void. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] xndai edited a comment on issue #1740: [CALCITE-3713] Remove column names from Project#digest
xndai edited a comment on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584911960 >Every time we create a new RelType we build stings. Every time we create RelNode we build strings. Every time we create RexNode we build strings. Yes, but if not string, you would be creating something else. I am fine with the initial overhead. But we cannot afford recomputing hash code every time when access the map. Unfortunately this is what `List` does with your change. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1799: [CALCITE-3785] HepPlanner.belongToDag() doesn't have to use mapDigest…
hsyuan commented on a change in pull request #1799: [CALCITE-3785]
HepPlanner.belongToDag() doesn't have to use mapDigest…
URL: https://github.com/apache/calcite/pull/1799#discussion_r377961442
##
File path: core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java
##
@@ -500,8 +500,7 @@ private void applyRules(
/** Returns whether the vertex is valid. */
private boolean belongsToDag(HepRelVertex vertex) {
-Pair> key = key(vertex.getCurrentRel());
-return mapDigestToVertex.get(key) != null;
+return graph.vertexSet().contains(vertex);
Review comment:
This method can be removed, we can call this directly in the caller.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xndai commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
xndai commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584911960 >Every time we create a new RelType we build stings. Every time we create RelNode we build strings. Every time we create RexNode we build strings. Yes, but if not string, you would be creating something else. I am fine with the initial overhead. But we cannot afford recomputing hash code every time when access the map. Unfortunately this is what List does with your change. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] xndai commented on issue #1799: [CALCITE-3785] HepPlanner.belongToDag() doesn't have to use mapDigest…
xndai commented on issue #1799: [CALCITE-3785] HepPlanner.belongToDag() doesn't have to use mapDigest… URL: https://github.com/apache/calcite/pull/1799#issuecomment-584905669 @hsyuan @vlsi This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] xndai opened a new pull request #1799: [CALCITE-3785] HepPlanner.belongToDag() doesn't have to use mapDigest…
xndai opened a new pull request #1799: [CALCITE-3785] HepPlanner.belongToDag() doesn't have to use mapDigest… URL: https://github.com/apache/calcite/pull/1799 …ToVertex To test whether or not a vertex belongs to DAG, we can simply do graph.vertexSet().contains(vertex). There's no need to look up in mapDigestToVertex map, which incurs overhead of creating the map key. This problem was amplified by CALCITE-3713. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] asereda-gs opened a new pull request #1798: [CALCITE-2442] Remove .toDelete cassandra temp folder on Windows after tests
asereda-gs opened a new pull request #1798: [CALCITE-2442] Remove .toDelete cassandra temp folder on Windows after tests URL: https://github.com/apache/calcite/pull/1798 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584851082 I wonder what @julianhyde and @zabetak think of https://github.com/apache/calcite/pull/1740#issuecomment-584848797 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584850203 > We don't build strings over and over Every time we create a new RelType we build stings. Every time we create RelNode we build strings. Every time we create RexNode we build strings. This is relevant: https://issues.apache.org/jira/browse/CALCITE-3784 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
vlsi commented on issue #1740: [CALCITE-3713] Remove column names from
Project#digest
URL: https://github.com/apache/calcite/pull/1740#issuecomment-584848797
> second string is the new digest
The use of stings for digests is unfortunate. It takes time to build a
string, however in practice we don't need that as a single object.
What I mean is something behind the lines of
```java
class RelDataType {
Digest digest;
Digest getDigestsSansFieldNames() {
Digest digest = this.digset;
if (digest != null) return digest;
this.digest = digest = ...;
return digest;
}
}
class Digest {
final int hashCode;
final Object[] contents;
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] xndai commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
xndai commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584839764 > I would rather add a method to RelDataType that computes the digest without taking column names to the consideration. That would work. Then mapDigestToRel key would be Pair where the second string is the new digest without column name. It won't solve the memory growth problem though. > here we build strings again and again, and it might help if we would be able to skip building strings. Not sure what you mean. RelNode caches digest until recomputeDigest() is called. We don't build strings over and over. There might be a better abstraction to represent digest, but I think string serves the purpose currently. It would be a big change to use something else, and I don't see clear benefits. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584799605 @xndai , I would rather add a method to `RelDataType` that computes the digest without taking column names to the consideration. Note: we add an opaque `Digest` interface (or even `Object`), so it does not tie to `String`. It looks like we have similar issues with `RexNode` digest, and `RelNode` digest. There we build strings again and again, and it might help if we would be able to skip building strings. I don't insist that adding `Digestable` interface is the only way to go here, however, I think it is a nice opportunity, and it might be helpful for other digests as well. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] xndai commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
xndai commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584783530 Now the problem is every time when we access mapDigestToRel, we would have to recompute the hash code of List, which is a considerable overhead. It not only affects Hep planner, but also VolcanoPlanner. We could have another member variable in RelRecordType for digest without column name. Then somehow expose it as the hash string for a wrapper class on top of RelRecordType. Then mapToDigest would become Map, HepRelVertex>. This will solve the problem of re-hasing, but it increase the memory usage by having an additional digest member. I think the best way would be removing column name from RelRecordType digest. I understand it breaks the UT currently, but we should spend time understanding why if we believe this's the right solution. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
vlsi edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter URL: https://github.com/apache/calcite/pull/1792#issuecomment-584757275 >But the Schema interface is important to end-users. Modifying the interface may bring impact on compatibility. There are ways to do that without breaking compatibility much: a) Release Calcite 2.0 b) Add a method with `default` implementation Both of the options are better than adding a half-baked solution to SimpleCalciteSchema. > SimpleCalciteSchema needs to **_construct the data structure_** each time. I do not see that documented. Can you please pin-point? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
vlsi edited a comment on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter URL: https://github.com/apache/calcite/pull/1792#issuecomment-584757275 >But the Schema interface is important to end-users. Modifying the interface may bring impact on compatibility. There are ways to do that without breaking compatibility much: a) Release Calcite 2.0 b) Add a method with `default` implementation Both of the options are better than adding a half-baked solution to SimpleCalciteSchema. > SimpleCalciteSchema needs to **construct the data structure** each time. I do not see that documented. Can you please pin-point? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
vlsi commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter URL: https://github.com/apache/calcite/pull/1792#issuecomment-584757275 >But the Schema interface is important to end-users. Modifying the interface may bring impact on compatibility. There are ways to do that without breaking compatibility much: a) Release Calcite 2.0 b) Add a method with `default` implementation Both of the options are better than adding a half-baked solution to SimpleCalciteSchema. > SimpleCalciteSchema needs to construct the data structure each time. I do not see that documented. Can you please pin-point? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584754624 Just in case. Removal of the field names from `RelRecordType::generateTypeString` produces extreme amount of test failures: https://github.com/apache/calcite/pull/1797 So it does not look like we can make such a change. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] DonnyZone commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter URL: https://github.com/apache/calcite/pull/1792#issuecomment-584669334 In the respect of performance, I agree with you to argument `Schema` interface with case (in)sensitive lookups. This can benefit not only `SimpleCalciteSchema` but also `CachingCalciteSchema`. But the `Schema` interface is important to end-users. Modifying the interface may bring impact on compatibility. Moreover, I don't think the performance of adopting `NamedMap` is better than current PR's implementation's `O(n)` complexity . According to the design doc (without caching, "maintains minimal state"), `SimpleCalciteSchema` needs to construct the data structure each time. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] vlsi opened a new pull request #1797: WIP: Remove field#name from RelRecordType#digest
vlsi opened a new pull request #1797: WIP: Remove field#name from RelRecordType#digest URL: https://github.com/apache/calcite/pull/1797 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] rubenada opened a new pull request #1796: [CALCITE-3783] PruneEmptyRules#JOIN_RIGHT_INSTANCE wrong behavior for JoinRelType.ANTI
rubenada opened a new pull request #1796: [CALCITE-3783] PruneEmptyRules#JOIN_RIGHT_INSTANCE wrong behavior for JoinRelType.ANTI URL: https://github.com/apache/calcite/pull/1796 Jira ticket: [CALCITE-3783](https://issues.apache.org/jira/browse/CALCITE-3783) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[calcite-site] branch master updated: Site: Update IntelliJ instructions with suggested and problematic versions
This is an automated email from the ASF dual-hosted git repository. zabetak pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/calcite-site.git The following commit(s) were added to refs/heads/master by this push: new 7cc41be Site: Update IntelliJ instructions with suggested and problematic versions 7cc41be is described below commit 7cc41be94889abc2ff9d39e40db1e8edd11d5795 Author: Stamatis Zampetakis AuthorDate: Tue Feb 11 14:10:06 2020 +0100 Site: Update IntelliJ instructions with suggested and problematic versions --- docs/howto.html | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/howto.html b/docs/howto.html index fe9c36c..14b9b8f 100644 --- a/docs/howto.html +++ b/docs/howto.html @@ -321,7 +321,11 @@ Integration tests should be named ...IT.javaSetting up IntelliJ IDEA -To setup https://www.jetbrains.com/idea/;>IntelliJ IDEA, follow the standard steps for the installation of IDEA and set up one of the JDK versions currently supported by Calcite. +Download a version of https://www.jetbrains.com/idea/;>IntelliJ IDEA greater than (2018.X). Versions 2019.2, and +2019.3 have been tested by members of the community and appear to be stable. Older versions of IDEA may still work +without problems for Calcite sources that do not use the Gradle build (release 1.21.0 and before). + +Follow the standard steps for the installation of IDEA and set up one of the JDK versions currently supported by Calcite. Start with building Calcite from the command line.
[calcite] branch master updated (e89e18d -> 15fe434)
This is an automated email from the ASF dual-hosted git repository. zabetak pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git. from e89e18d Update Gradle test output formatting add 15fe434 Site: Update IntelliJ instructions with suggested and problematic versions No new revisions were added by this update. Summary of changes: site/_docs/howto.md | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-)
[GitHub] [calcite] vlsi commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
vlsi commented on issue #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter URL: https://github.com/apache/calcite/pull/1792#issuecomment-584577513 I think we should better augment Schema interface with `case insensitive` lookups. Most of the implementations should be able to delegate case insensitive requests to NameMap, which would be OK for performance. The implementation in the current PR is sad for performance. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377542184
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Thanks! I see, ignore the above comment.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest
vlsi commented on issue #1740: [CALCITE-3713] Remove column names from Project#digest URL: https://github.com/apache/calcite/pull/1740#issuecomment-584548560 @xndai , I just thought removal of column name from `RelRecordType::generateTypeString` might have non-trivial side effects for Calcite consumers. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] DonnyZone commented on issue #1405: [CALCITE-2772] Support varargs UDF for scalar function
DonnyZone commented on issue #1405: [CALCITE-2772] Support varargs UDF for scalar function URL: https://github.com/apache/calcite/pull/1405#issuecomment-584546905 Thanks @zabetak, I have not ever tried the fix in our environment. But the PR is OK to me. I do not have any further comments. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377520403
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Could you elaborate it? Or do you mean enumerating all possibilities?
E.g., converting `getTable("xy", false) ` to
`getTable("XY") + getTable("Xy") + getTable("xY") +getTable("xy")`?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] zabetak commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
zabetak commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377496849
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
I would go one step further and not even call `getTableNames()` etc., if it
is not necessary. In some cases `schema.getTableNames()` may be constructed
each time on demand.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] zabetak commented on issue #1405: [CALCITE-2772] Support varargs UDF for scalar function
zabetak commented on issue #1405: [CALCITE-2772] Support varargs UDF for scalar function URL: https://github.com/apache/calcite/pull/1405#issuecomment-584524564 It seems that @gr4ve adressed the comments of @danny0405 . @DonnyZone , @XuQianJin-Stars do you believe this PR is ready for merge? If yes then I can do the rebase and solve any remaining minor glithces (if there are). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [calcite] wangxlong opened a new pull request #1795: [CALCITE-3779]Implement bitand scalar function
wangxlong opened a new pull request #1795: [CALCITE-3779]Implement bitand scalar function URL: https://github.com/apache/calcite/pull/1795 Implement bitand scalar function which support tinyint, smallint, int, bigint, binary and varbinary type. Related issue: [CALCITE-3779](https://issues.apache.org/jira/browse/CALCITE-3779) Parent issue: [CALCITE-3732](https://issues.apache.org/jira/browse/CALCITE-3732) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
