korlov42 commented on code in PR #4927:
URL: https://github.com/apache/ignite-3/pull/4927#discussion_r1895383601
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ScannableTableImpl.java:
##########
@@ -221,7 +221,7 @@ public <RowT> Publisher<RowT> indexLookup(
@Override
public <RowT> CompletableFuture<RowT> primaryKeyLookup(
ExecutionContext<RowT> ctx,
- @Nullable InternalTransaction explicitTx,
+ InternalTransaction tx,
Review Comment:
I think, it worth to add assertion making sure no-one is passing `null`
instead of transaction
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/QueryTransactionContext.java:
##########
@@ -24,8 +24,15 @@
* Context that allows to get explicit transaction provided by user or start
implicit one.
*/
public interface QueryTransactionContext {
- /** Returns explicit transaction or start implicit one. */
- QueryTransactionWrapper getOrStartImplicit(boolean readOnly);
+ /**
+ * Starts an implicit transaction if one has not been started previously
+ * and if there is no external (user-managed) transaction.
+ *
+ * @param readOnly Indicates whether the read-only transaction or
read-write transaction should be started.
+ * @param tableDriven Indicates whether the implicit transaction will be
managed by the table storage or the SQL engine.
+ * @return Transaction wrapper.
+ */
+ QueryTransactionWrapper getOrStartImplicit(boolean readOnly, boolean
tableDriven);
Review Comment:
perhaps, it would be better to change names to
`getOrStartSqlManaged(readOnly, implicit)`, WDYT?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/KeyValueGetPlan.java:
##########
@@ -142,60 +148,65 @@ public <RowT> AsyncCursor<InternalSqlRow> execute(
List<RexNode> projectionExpr = lookupNode.projects();
List<RexNode> keyExpressions = lookupNode.keyExpressions();
- RelDataType rowType = sqlTable.getRowType(Commons.typeFactory(),
requiredColumns);
-
- Supplier<RowT> keySupplier = ctx.expressionFactory()
- .rowSource(keyExpressions);
- Predicate<RowT> filter = filterExpr == null ? null :
ctx.expressionFactory()
- .predicate(filterExpr, rowType);
- Function<RowT, RowT> projection = projectionExpr == null ? null :
ctx.expressionFactory()
- .project(projectionExpr, rowType);
-
- RowHandler<RowT> rowHandler = ctx.rowHandler();
- RowSchema rowSchema =
TypeUtils.rowSchemaFromRelTypes(RelOptUtil.getFieldTypeList(rowType));
- RowFactory<RowT> rowFactory = rowHandler.factory(rowSchema);
-
- RelDataType resultType = lookupNode.getRowType();
- BiFunction<Integer, Object, Object> internalTypeConverter =
TypeUtils.resultTypeConverter(ctx, resultType);
-
- ScannableTable scannableTable = execTable.scannableTable();
- Function<RowT, Iterator<InternalSqlRow>> postProcess = row -> {
- if (row == null) {
- return Collections.emptyIterator();
- }
-
- if (filter != null && !filter.test(row)) {
- return Collections.emptyIterator();
- }
+ CompletableFuture<Iterator<InternalSqlRow>> result;
- if (projection != null) {
- row = projection.apply(row);
+ try {
Review Comment:
does it make sense to move this `try-catch` on `ExecutionServiceImpl` level?
This way we cover all the problem in a single place
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -442,9 +443,10 @@ private AsyncDataCursor<InternalSqlRow>
executeExecutablePlan(
QueryTransactionWrapper txWrapper = txContext.explicitTx();
if (txWrapper == null) {
- // underlying table will initiate transaction by itself, but we
need stub to reuse
- // TxAwareAsyncCursor
- txWrapper = NoopTransactionWrapper.INSTANCE;
+ txWrapper = plan.transactional()
Review Comment:
the only "non-transactional" plan is `SelectCountPlan` which is more like
temporal solution. I would prefer not to introduce `transactional` property on
`ExecutablePlan` interface, and get rid of `NoopTransactionWrapper` instead
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
