IGNITE-12692 SQL Calcite: Distributed table modify #12593
Conversation
alex-plekhanov
force-pushed
the
ignite-12692
branch
from
8ad2bb1 to
a523525
Compare
alex-plekhanov
force-pushed
the
ignite-12692
branch
from
a523525 to
e5116e8
Compare
...alcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/type/OtherType.java
Outdated
Show resolved
Hide resolved
|
|
|
||
| /** | ||
| * Creates a ConverterRule. | ||
| */ | ||
| public TableModifyConverterRule() { |
There was a problem hiding this comment.
Suggestion. Creates TableModifySingleNodeConverterRule
| } | ||
|
|
||
| /** {@inheritDoc} */ | ||
| @Override protected @Nullable PhysicalNode convert( |
There was a problem hiding this comment.
Trivial codestyle. Might be a sigle line. Up to you.
|
|
||
| switch (rel.getOperation()) { | ||
| case MERGE: | ||
| // Merge contains insert fields as well as update fields, it's impossible to check input distribution |
There was a problem hiding this comment.
Trivial spelling. Lets fix the sentences. Use . instead of ,.
|
|
||
| RelDataType type = fields[i + 1] instanceof Class ? TYPE_FACTORY.createJavaType((Class<?>)fields[i + 1]) : | ||
| TYPE_FACTORY.createSqlType((SqlTypeName)fields[i + 1]); | ||
| TYPE_FACTORY.createTypeWithNullability(TYPE_FACTORY.createSqlType((SqlTypeName)fields[i + 1]), true); |
There was a problem hiding this comment.
Suggestion, lets split the lines like:
RelDataType type = fields[i + 1] instanceof Class
? TYPE_FACTORY.createJavaType((Class<?>)fields[i + 1])
: TYPE_FACTORY.createTypeWithNullability(TYPE_FACTORY.createSqlType((SqlTypeName)fields[i + 1]), true);
| return null; | ||
|
|
||
| switch (rel.getOperation()) { | ||
| case MERGE: |
There was a problem hiding this comment.
Do we have tests for this case?
| // Check UPDATE statements. | ||
|
|
||
| // partitioned. | ||
| assertPlan("UPDATE test SET val = val + 1", schema, |
There was a problem hiding this comment.
It looks really too short check. Maybe we need to search for Excnanheg/Aggregates.
|
|
||
| // partitioned. | ||
| assertPlan("UPDATE test SET val = val + 1", schema, | ||
| hasChildThat(isInstanceOf(IgniteTableModify.class).and(hasDistribution(IgniteDistributions.random())) |
There was a problem hiding this comment.
Why random(), not hash() ?
| * Converts LogicalTableModify to distributed IgniteTableModify (Perform table modify on remote nodes, | ||
| * aggregate affected rows count and send result to the initiator node). | ||
| */ | ||
| public class TableModifyDistributedConverterRule extends AbstractIgniteConverterRule<LogicalTableModify> { |
There was a problem hiding this comment.
What if underlying cache has a node filer? Do we need to consider it?
| .and(input(isTableScan("TEST_REPL")))); | ||
|
|
||
| // broadcast (force distributed). | ||
| assertPlan("UPDATE test_repl SET val = val + 1", schema, |
There was a problem hiding this comment.
Can it duplicate or corrupt data? Or TrimExchange prevents?
|
|
||
| /** Tests that table modify can be executed on remote nodes. */ | ||
| @Test | ||
| public void testDistributedTableModify() throws Exception { |
There was a problem hiding this comment.
Do we need tests with filters (WHERE )
2 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.