You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "zstan (via GitHub)" <gi...@apache.org> on 2023/06/12 12:18:00 UTC
[GitHub] [ignite-3] zstan opened a new pull request, #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
zstan opened a new pull request, #2181:
URL: https://github.com/apache/ignite-3/pull/2181
…T statements without FROM keyword, change mapping
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#issuecomment-1589237743
@korlov42 i revert tx fix and change only mapping part, can u take a look ?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228113151
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##########
@@ -85,33 +231,19 @@ public MetadataDef<FragmentMappingMetadata> getDef() {
* @param mq Metadata query instance. Used to request appropriate metadata from node children.
* @return Nodes mapping, representing a list of nodes capable to execute a query over particular partitions.
*/
- public FragmentMapping fragmentMapping(RelNode rel, RelMetadataQuery mq, MappingQueryContext ctx) {
- throw new AssertionError();
- }
-
- /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
- */
- public FragmentMapping fragmentMapping(RelSubset rel, RelMetadataQuery mq, MappingQueryContext ctx) {
- throw new AssertionError();
- }
-
- /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
- */
- public FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
+ private static FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
return fragmentMappingForMetadataQuery(rel.getInput(), mq, ctx);
}
/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
+ * See {@link IgniteFragmentMapping#fragmentMapping(SingleRel, RelMetadataQuery, MappingQueryContext)}.
*
* <p>{@link ColocationMappingException} may be thrown on two children nodes locations merge. This means that the fragment
* (which part the parent node is) cannot be executed on any node and additional exchange is needed. This case we throw {@link
* NodeMappingException} with an edge, where we need the additional exchange. After the exchange is put into the fragment and the
* fragment is split into two ones, fragment meta information will be recalculated for all fragments.
*/
- public FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
+ private static FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
Review Comment:
why do we need all those static fields? It's better to inline them
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228113886
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##########
@@ -69,13 +75,153 @@ public class IgniteMdFragmentMapping implements MetadataHandler<FragmentMappingM
public static FragmentMapping fragmentMappingForMetadataQuery(RelNode rel, RelMetadataQuery mq, MappingQueryContext ctx) {
Review Comment:
it worth to rename this method
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228370294
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##########
@@ -85,33 +231,19 @@ public MetadataDef<FragmentMappingMetadata> getDef() {
* @param mq Metadata query instance. Used to request appropriate metadata from node children.
* @return Nodes mapping, representing a list of nodes capable to execute a query over particular partitions.
*/
- public FragmentMapping fragmentMapping(RelNode rel, RelMetadataQuery mq, MappingQueryContext ctx) {
- throw new AssertionError();
- }
-
- /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
- */
- public FragmentMapping fragmentMapping(RelSubset rel, RelMetadataQuery mq, MappingQueryContext ctx) {
- throw new AssertionError();
- }
-
- /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
- */
- public FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
+ private static FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
return fragmentMappingForMetadataQuery(rel.getInput(), mq, ctx);
}
/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
+ * See {@link IgniteFragmentMapping#fragmentMapping(SingleRel, RelMetadataQuery, MappingQueryContext)}.
*
* <p>{@link ColocationMappingException} may be thrown on two children nodes locations merge. This means that the fragment
* (which part the parent node is) cannot be executed on any node and additional exchange is needed. This case we throw {@link
* NodeMappingException} with an edge, where we need the additional exchange. After the exchange is put into the fragment and the
* fragment is split into two ones, fragment meta information will be recalculated for all fragments.
*/
- public FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
+ private static FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
Review Comment:
we can inline them only partially, for example
`fragmentMapping(BiRel rel` it\`s about CorrJoin, NestedJoin and LoopJoin too
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#discussion_r1228370294
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/metadata/IgniteFragmentMapping.java:
##########
@@ -85,33 +231,19 @@ public MetadataDef<FragmentMappingMetadata> getDef() {
* @param mq Metadata query instance. Used to request appropriate metadata from node children.
* @return Nodes mapping, representing a list of nodes capable to execute a query over particular partitions.
*/
- public FragmentMapping fragmentMapping(RelNode rel, RelMetadataQuery mq, MappingQueryContext ctx) {
- throw new AssertionError();
- }
-
- /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
- */
- public FragmentMapping fragmentMapping(RelSubset rel, RelMetadataQuery mq, MappingQueryContext ctx) {
- throw new AssertionError();
- }
-
- /**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
- */
- public FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
+ private static FragmentMapping fragmentMapping(SingleRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
return fragmentMappingForMetadataQuery(rel.getInput(), mq, ctx);
}
/**
- * See {@link IgniteMdFragmentMapping#fragmentMapping(RelNode, RelMetadataQuery, MappingQueryContext)}.
+ * See {@link IgniteFragmentMapping#fragmentMapping(SingleRel, RelMetadataQuery, MappingQueryContext)}.
*
* <p>{@link ColocationMappingException} may be thrown on two children nodes locations merge. This means that the fragment
* (which part the parent node is) cannot be executed on any node and additional exchange is needed. This case we throw {@link
* NodeMappingException} with an edge, where we need the additional exchange. After the exchange is put into the fragment and the
* fragment is split into two ones, fragment meta information will be recalculated for all fragments.
*/
- public FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
+ private static FragmentMapping fragmentMapping(BiRel rel, RelMetadataQuery mq, MappingQueryContext ctx) {
Review Comment:
we can inline them only partially, for example
`fragmentMapping(BiRel rel` it\`s about CorrJoin, NestedJoin and MergeJoin too
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181#issuecomment-1587358217
@korlov42 can u make a review ? There are exist some questionable places as for me.
It`s not clear question - can Boolean hold ternary logic or only binary, i think it can )
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan merged pull request #2181: IGNITE-19709 Sql. Get rid of starting implicit transactions for SELECT statements without FROM keyword, change mapping
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan merged PR #2181:
URL: https://github.com/apache/ignite-3/pull/2181
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org