You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/05/13 21:13:51 UTC
[GitHub] [calcite] hsyuan opened a new pull request #1976: Add TransformationRule interface
hsyuan opened a new pull request #1976:
URL: https://github.com/apache/calcite/pull/1976
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628858500
The restriction only applies operator that implements `PhysicalNode`, which is a newly introduced interface, no one has the chance to implement it until 1.23.0 is released.
see https://github.com/apache/calcite/commit/7be30db36d449e0a7fcc76b7d4647e141f4bc72d#diff-70d5035797baebe157bab8cbf860e456R415
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628802576
If it can match physical operators, that means it is already matches with logical operators, it just generates redundant operators. What scenario will it break?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628739137
There won't be conflicts. It is the thorough fix.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
xndai commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628798575
If this's something we believe should be enforced, why don't we just add such check?
There are a number of rules you can be applied to both logical and physical nodes today (matching the base rel class instead of the logical rel). Those rules won't fire anymore with your change. Certain scenarios could break due to this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
xndai commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628816637
If one physical node is created by a rule, rather than being converted from a logical node, then there's no logical counterpart and won't be processed by your transformation rule. We don't have such rule in Calcite, but it doesn't mean the user cannot create one.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628834771
The only possible case is the user's special rule creates a physical operator that is a sub-class of Enumerable operator, at the same use Calcite's built-in logical rule to process it.
Sounds rare but fare, I will add it in the release notes.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] hsyuan closed pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
hsyuan closed pull request #1976:
URL: https://github.com/apache/calcite/pull/1976
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
xndai commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628783691
How do you guarantee only logical nodes are generated in transformation rule? Also please make sure to include this in the release note as a potential breaking 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:
users@infra.apache.org
[GitHub] [calcite] eolivelli commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628737002
@hsyuan thank you !
@vlsi
Do you know how to build locally this branch and put the jars into local Maven repository ?
this way I can test this branch against my branch on HerdDB
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] xndai commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
xndai commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628856256
Not necessarily a sub-class of Enumerable. It could be a sub-class of the base RelNode, such as Project or Aggregate.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
hsyuan commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628792593
> How do you guarantee only logical nodes are generated in transformation rule?
It is allowed, but not encouraged. Just a contract, unless we add a check inside `call.transformTo`, which can be added in next version.
> Also please make sure to include this in the release note as a potential breaking change.
What do you think it will break?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] eolivelli commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628738883
btw I see now that you added a marker interface.
It is a big change.
But I don't have enough context in order to give a suggestion
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [calcite] eolivelli commented on pull request #1976: [CALCITE-3997] Logical rules matched with physical operators but failed to handle traits
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1976:
URL: https://github.com/apache/calcite/pull/1976#issuecomment-628737545
@hsyuan it looks like you modified 92 files...
IMHO it would be better to narrow down the patch to the minimal set of changes, in order to prevent conflicts with other patches
----------------------------------------------------------------
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:
users@infra.apache.org