This is an automated email from the ASF dual-hosted git repository. stoty pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/phoenix.git
The following commit(s) were added to refs/heads/master by this push: new b248756 PHOENIX-6665 PreparedStatement#getMetaData() fails on parametrized "select next ? values for SEQ" b248756 is described below commit b2487566934c4749d21e7f9d34c44cd925b94f07 Author: Istvan Toth <st...@apache.org> AuthorDate: Mon Mar 21 06:44:47 2022 +0100 PHOENIX-6665 PreparedStatement#getMetaData() fails on parametrized "select next ? values for SEQ" --- .../phoenix/end2end/SequenceBulkAllocationIT.java | 18 ++--- .../apache/phoenix/compile/SequenceManager.java | 82 +++++++++++++--------- .../phoenix/compile/SequenceValueExpression.java | 25 +++++-- .../phoenix/jdbc/PhoenixPreparedStatement.java | 4 +- .../java/org/apache/phoenix/schema/Sequence.java | 6 +- 5 files changed, 84 insertions(+), 51 deletions(-) diff --git a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SequenceBulkAllocationIT.java b/phoenix-core/src/it/java/org/apache/phoenix/end2end/SequenceBulkAllocationIT.java index 489513a..bac1dec 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/end2end/SequenceBulkAllocationIT.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/end2end/SequenceBulkAllocationIT.java @@ -1165,15 +1165,15 @@ public class SequenceBulkAllocationIT extends ParallelStatsDisabledIT { .executeQuery( "SELECT start_with, current_value, increment_by, cache_size, min_value, max_value, cycle_flag, sequence_schema, sequence_name FROM \"SYSTEM\".\"SEQUENCE\" where sequence_name='" + props.getNameWithoutSchema() + "'"); assertTrue(rs.next()); - assertEquals(props.startsWith, rs.getLong("start_with")); - assertEquals(props.incrementBy, rs.getLong("increment_by")); - assertEquals(props.cacheSize, rs.getLong("cache_size")); - assertEquals(false, rs.getBoolean("cycle_flag")); - assertEquals(props.getSchemaName(), rs.getString("sequence_schema")); - assertEquals(props.getNameWithoutSchema(), rs.getString("sequence_name")); - assertEquals(currentValue, rs.getLong("current_value")); - assertEquals(props.minValue, rs.getLong("min_value")); - assertEquals(props.maxValue, rs.getLong("max_value")); + assertEquals("start_with", props.startsWith, rs.getLong("start_with")); + assertEquals("increment_by", props.incrementBy, rs.getLong("increment_by")); + assertEquals("cache_size", props.cacheSize, rs.getLong("cache_size")); + assertEquals("cycle_flag", false, rs.getBoolean("cycle_flag")); + assertEquals("sequence_schema", props.getSchemaName(), rs.getString("sequence_schema")); + assertEquals("sequence_name", props.getNameWithoutSchema(), rs.getString("sequence_name")); + assertEquals("current_value", currentValue, rs.getLong("current_value")); + assertEquals("min_value", props.minValue, rs.getLong("min_value")); + assertEquals("max_value", props.maxValue, rs.getLong("max_value")); assertFalse(rs.next()); } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceManager.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceManager.java index c19b3cb..0f50443 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceManager.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceManager.java @@ -27,6 +27,7 @@ import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.io.ImmutableBytesWritable; import org.apache.phoenix.exception.SQLExceptionCode; import org.apache.phoenix.expression.Expression; +import org.apache.phoenix.expression.LiteralExpression; import org.apache.phoenix.jdbc.PhoenixStatement; import org.apache.phoenix.parse.ParseNode; import org.apache.phoenix.parse.SequenceValueParseNode; @@ -50,9 +51,9 @@ public class SequenceManager { private int[] sequencePosition; private List<SequenceAllocation> nextSequences; private List<SequenceKey> currentSequences; - private final Map<SequenceKey,SequenceValueExpression> sequenceMap = Maps.newHashMap(); + private final Map<SequenceKey, SequenceValueExpression> sequenceMap = Maps.newHashMap(); private final BitSet isNextSequence = new BitSet(); - + public SequenceManager(PhoenixStatement statement) { this.statement = statement; } @@ -130,64 +131,78 @@ public class SequenceManager { int nSaltBuckets = statement.getConnection().getQueryServices().getSequenceSaltBuckets(); ParseNode numToAllocateNode = node.getNumToAllocateNode(); - long numToAllocate = determineNumToAllocate(tableName, numToAllocateNode); + Expression numToAllocateExp = numToAllocateExpression(tableName, numToAllocateNode); SequenceKey key = new SequenceKey(tenantId, tableName.getSchemaName(), tableName.getTableName(), nSaltBuckets); + SequenceValueExpression expression = sequenceMap.get(key); if (expression == null) { int index = sequenceMap.size(); - expression = new SequenceValueExpression(key, node.getOp(), index, numToAllocate); - sequenceMap.put(key, expression); - } else if (expression.op != node.getOp() || expression.getNumToAllocate() < numToAllocate) { - // Keep the maximum allocation size we see in a statement + expression = new SequenceValueExpression(key, node.getOp(), index, numToAllocateExp); + } else { + // Add the new numToAllocateExp to the expression SequenceValueExpression oldExpression = expression; - expression = new SequenceValueExpression(key, node.getOp(), expression.getIndex(), Math.max(expression.getNumToAllocate(), numToAllocate)); - if (oldExpression.getNumToAllocate() < numToAllocate) { - // If we found a NEXT VALUE expression with a higher number to allocate - // We override the original expression - sequenceMap.put(key, expression); - } - } + expression = new SequenceValueExpression(oldExpression, node.getOp(), numToAllocateExp); + } + sequenceMap.put(key, expression); + // If we see a NEXT and a CURRENT, treat the CURRENT just like a NEXT if (node.getOp() == Op.NEXT_VALUE) { isNextSequence.set(expression.getIndex()); } - + return expression; } + private Expression numToAllocateExpression(TableName tableName, ParseNode numToAllocateNode) throws SQLException { + if (numToAllocateNode != null) { + final StatementContext context = new StatementContext(statement); + ExpressionCompiler expressionCompiler = new ExpressionCompiler(context); + return numToAllocateNode.accept(expressionCompiler); + } else { + // Standard Sequence Allocation Behavior + return LiteralExpression.newConstant(SequenceUtil.DEFAULT_NUM_SLOTS_TO_ALLOCATE); + } + } + /** * If caller specified used NEXT <n> VALUES FOR <seq> expression then we have set the numToAllocate. * If numToAllocate is > 1 we treat this as a bulk reservation of a block of sequence slots. * - * @throws a SQLException if we can't compile the expression + * @throws a SQLException if we can't evaluate the expression */ - private long determineNumToAllocate(TableName sequenceName, ParseNode numToAllocateNode) + private long determineNumToAllocate(SequenceValueExpression expression) throws SQLException { - if (numToAllocateNode != null) { - final StatementContext context = new StatementContext(statement); - ExpressionCompiler expressionCompiler = new ExpressionCompiler(context); - Expression expression = numToAllocateNode.accept(expressionCompiler); + final StatementContext context = new StatementContext(statement); + long maxNumToAllocate = 0; + for (Expression numToAllocateExp : expression.getNumToAllocateExpressions()) { ImmutableBytesWritable ptr = context.getTempPtr(); - expression.evaluate(null, ptr); - if (ptr.getLength() == 0 || !expression.getDataType().isCoercibleTo(PLong.INSTANCE)) { - throw SequenceUtil.getException(sequenceName.getSchemaName(), sequenceName.getTableName(), SQLExceptionCode.NUM_SEQ_TO_ALLOCATE_MUST_BE_CONSTANT); + numToAllocateExp.evaluate(null, ptr); + if (ptr.getLength() == 0 || !numToAllocateExp.getDataType().isCoercibleTo(PLong.INSTANCE)) { + throw SequenceUtil.getException(expression.getKey().getSchemaName(), + expression.getKey().getSequenceName(), + SQLExceptionCode.NUM_SEQ_TO_ALLOCATE_MUST_BE_CONSTANT); } // Parse <n> and make sure it is greater than 0. We don't support allocating 0 or negative values! - long numToAllocate = (long) PLong.INSTANCE.toObject(ptr, expression.getDataType()); + long numToAllocate = (long) PLong.INSTANCE.toObject(ptr, numToAllocateExp.getDataType()); if (numToAllocate < 1) { - throw SequenceUtil.getException(sequenceName.getSchemaName(), sequenceName.getTableName(), SQLExceptionCode.NUM_SEQ_TO_ALLOCATE_MUST_BE_CONSTANT); + throw SequenceUtil.getException(expression.getKey().getSchemaName(), + expression.getKey().getSequenceName(), + SQLExceptionCode.NUM_SEQ_TO_ALLOCATE_MUST_BE_CONSTANT); + } + if (numToAllocate > maxNumToAllocate) { + maxNumToAllocate = numToAllocate; } - return numToAllocate; - - } else { - // Standard Sequence Allocation Behavior - return SequenceUtil.DEFAULT_NUM_SLOTS_TO_ALLOCATE; } + + return maxNumToAllocate; } - + public void validateSequences(Sequence.ValueOp action) throws SQLException { + if (action == Sequence.ValueOp.NOOP) { + return; + } if (sequenceMap.isEmpty()) { return; } @@ -198,7 +213,8 @@ public class SequenceManager { currentSequences = Lists.newArrayListWithExpectedSize(maxSize); for (Map.Entry<SequenceKey, SequenceValueExpression> entry : sequenceMap.entrySet()) { if (isNextSequence.get(entry.getValue().getIndex())) { - nextSequences.add(new SequenceAllocation(entry.getKey(), entry.getValue().getNumToAllocate())); + nextSequences.add(new SequenceAllocation(entry.getKey(), + determineNumToAllocate(entry.getValue()))); } else { currentSequences.add(entry.getKey()); } diff --git a/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceValueExpression.java b/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceValueExpression.java index 71e2d02..8ba3183 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceValueExpression.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/compile/SequenceValueExpression.java @@ -17,9 +17,14 @@ */ package org.apache.phoenix.compile; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + import org.apache.hadoop.hbase.io.ImmutableBytesWritable; import org.apache.phoenix.expression.BaseTerminalExpression; import org.apache.phoenix.expression.Determinism; +import org.apache.phoenix.expression.Expression; import org.apache.phoenix.expression.visitor.ExpressionVisitor; import org.apache.phoenix.parse.SequenceValueParseNode.Op; import org.apache.phoenix.schema.SequenceKey; @@ -32,17 +37,25 @@ public class SequenceValueExpression extends BaseTerminalExpression { private final SequenceKey key; final Op op; private final int index; - private final long numToAllocate; + private final Set<Expression> numToAllocateExpressions = new HashSet<>(); - public SequenceValueExpression(SequenceKey key, Op op, int index, long numToAllocate) { + public SequenceValueExpression(SequenceKey key, Op op, int index, Expression numToAllocateExp) { this.key = key; this.op = op; this.index = index; - this.numToAllocate = numToAllocate; + this.numToAllocateExpressions.add(numToAllocateExp); + } + + public SequenceValueExpression(SequenceValueExpression seqIn, Op op, Expression numToAllocateExp) { + this.key = seqIn.getKey(); + this.op = op; + this.index = seqIn.getIndex(); + this.numToAllocateExpressions.addAll(seqIn.numToAllocateExpressions); + this.numToAllocateExpressions.add(numToAllocateExp); } - public long getNumToAllocate() { - return numToAllocate; + public Set<Expression> getNumToAllocateExpressions() { + return numToAllocateExpressions; } public SequenceKey getKey() { @@ -83,7 +96,7 @@ public class SequenceValueExpression extends BaseTerminalExpression { @Override public String toString() { - return op.getName() + (numToAllocate == 1 ? " VALUE " : (" " + numToAllocate + " VALUES " )) + "FOR " + SchemaUtil.getTableName(key.getSchemaName(),key.getSequenceName()); + return op.getName() + Arrays.toString(getNumToAllocateExpressions().toArray()) + " VALUE(S) " + "FOR " + SchemaUtil.getTableName(key.getSchemaName(),key.getSequenceName()); } @Override diff --git a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixPreparedStatement.java b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixPreparedStatement.java index 2a13121..1b2b645 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixPreparedStatement.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixPreparedStatement.java @@ -222,7 +222,7 @@ public class PhoenixPreparedStatement extends PhoenixStatement implements Prepar } try { // Just compile top level query without optimizing to get ResultSetMetaData - QueryPlan plan = statement.compilePlan(this, Sequence.ValueOp.VALIDATE_SEQUENCE); + QueryPlan plan = statement.compilePlan(this, Sequence.ValueOp.NOOP); return new PhoenixResultSetMetaData(this.getConnection(), plan.getProjector()); } finally { int lastSetBit = 0; @@ -245,7 +245,7 @@ public class PhoenixPreparedStatement extends PhoenixStatement implements Prepar } } try { - StatementPlan plan = statement.compilePlan(this, Sequence.ValueOp.VALIDATE_SEQUENCE); + StatementPlan plan = statement.compilePlan(this, Sequence.ValueOp.NOOP); return plan.getParameterMetaData(); } finally { int lastSetBit = 0; diff --git a/phoenix-core/src/main/java/org/apache/phoenix/schema/Sequence.java b/phoenix-core/src/main/java/org/apache/phoenix/schema/Sequence.java index 515a99c..ae2a20a 100644 --- a/phoenix-core/src/main/java/org/apache/phoenix/schema/Sequence.java +++ b/phoenix-core/src/main/java/org/apache/phoenix/schema/Sequence.java @@ -63,7 +63,11 @@ import org.apache.phoenix.thirdparty.com.google.common.math.LongMath; public class Sequence { public static final int SUCCESS = 0; - public enum ValueOp {VALIDATE_SEQUENCE, INCREMENT_SEQUENCE}; + public enum ValueOp { + VALIDATE_SEQUENCE, // Check that the sequence statements are valid, during statement compilation + INCREMENT_SEQUENCE, // Perform the seqence operations, during execution + NOOP // Do not do anything, for compiling unbound prepared statements + }; public enum MetaOp {CREATE_SEQUENCE, DROP_SEQUENCE, RETURN_SEQUENCE}; // create empty Sequence key values used while created a sequence row