Details

    • Type: New Feature
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.8.0
    • Labels:
      None

      Description

      For serial query(query with “serial" hint or “limit" without "order by”), we can limit each scan(using page filter) to “limit+offset” instead of limit earlier.
      And then, for all queries, we can forward the relevant client iterators to the offset provided and then return the result.

      syntax

      [ LIMIT { count } ]
          [ OFFSET start [ ROW | ROWS ] ]
          [ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
      

      Some new keywords(OFFSET,FETCH,ROW, ROWS,ONLY) are getting introduced so users might need to see that they are not using them as column name or something.

      WDYT, James Taylor

      1. PHOENIX-2722.patch
        200 kB
        Ankit Singhal
      2. PHOENIX-2722_formatted.patch
        161 kB
        Ankit Singhal
      3. PHOENIX-2722_v1_rebased.patch
        188 kB
        Ankit Singhal

        Activity

        Hide
        vishal423 Vishal Mahajan added a comment -

        Can we also mention @since version in phoenix docs for this feature?

        Show
        vishal423 Vishal Mahajan added a comment - Can we also mention @since version in phoenix docs for this feature?
        Hide
        ankit@apache.org Ankit Singhal added a comment -

        Bulk close of all issues that has been resolved in a released version.

        Show
        ankit@apache.org Ankit Singhal added a comment - Bulk close of all issues that has been resolved in a released version.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal closed the pull request at:

        https://github.com/apache/phoenix/pull/154

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal closed the pull request at: https://github.com/apache/phoenix/pull/154
        Hide
        ankit.singhal Ankit Singhal added a comment -

        There seems to be some problem with Jenkins build as test cases are passing on my local machine.
        so committed to master and 4.x branches.

        Show
        ankit.singhal Ankit Singhal added a comment - There seems to be some problem with Jenkins build as test cases are passing on my local machine. so committed to master and 4.x branches.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Phoenix-master #1187 (See https://builds.apache.org/job/Phoenix-master/1187/)
        PHOENIX-2722 support mysql offset clause (ankitsinghal59: rev 776eea9ce1c4828f959b9ef76dd244941972f6ec)

        • phoenix-core/src/main/java/org/apache/phoenix/iterate/TableResultIterator.java
        • phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/DelegateQueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/SerialIterators.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/ClientProcessingPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/UnionPlan.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryWithOffsetIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/DeleteCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java
        • phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java
        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/util/QueryUtil.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/OffsetCompiler.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/HashJoinIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/JoinCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/parse/SelectStatement.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/SubqueryRewriter.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/TraceQueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
        • phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeRewriter.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java
        • phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/ScanPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java
        • phoenix-core/src/main/java/org/apache/phoenix/parse/DeleteStatement.java
        • phoenix-core/src/main/java/org/apache/phoenix/parse/FilterableStatement.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/LimitingResultIterator.java
        • phoenix-core/src/test/java/org/apache/phoenix/execute/LiteralResultIteratorPlanTest.java
        • phoenix-core/src/main/antlr3/PhoenixSQL.g
        • phoenix-core/src/main/java/org/apache/phoenix/parse/OffsetNode.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/SortMergeJoinPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/AggregatePlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/StatementNormalizer.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/ListJarsQueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/DegenerateQueryPlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/join/HashJoinInfo.java
        • phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
        • phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/ReadOnlyIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java
        • phoenix-core/src/main/java/org/apache/phoenix/compile/PostDDLCompiler.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetResultIterator.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/AutoCommitIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/ParallelIterators.java
        • phoenix-core/src/test/java/org/apache/phoenix/query/ParallelIteratorsSplitTest.java
        • phoenix-core/src/it/java/org/apache/phoenix/end2end/SortMergeJoinIT.java
        • phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedAggregatingResultIterator.java
        • phoenix-core/src/test/java/org/apache/phoenix/execute/UnnestArrayPlanTest.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Phoenix-master #1187 (See https://builds.apache.org/job/Phoenix-master/1187/ ) PHOENIX-2722 support mysql offset clause (ankitsinghal59: rev 776eea9ce1c4828f959b9ef76dd244941972f6ec) phoenix-core/src/main/java/org/apache/phoenix/iterate/TableResultIterator.java phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java phoenix-core/src/main/java/org/apache/phoenix/execute/DelegateQueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java phoenix-core/src/main/java/org/apache/phoenix/iterate/SerialIterators.java phoenix-core/src/main/java/org/apache/phoenix/execute/ClientProcessingPlan.java phoenix-core/src/main/java/org/apache/phoenix/execute/UnionPlan.java phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryWithOffsetIT.java phoenix-core/src/main/java/org/apache/phoenix/compile/DeleteCompiler.java phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java phoenix-core/src/it/java/org/apache/phoenix/end2end/CreateTableIT.java phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/util/QueryUtil.java phoenix-core/src/main/java/org/apache/phoenix/compile/OffsetCompiler.java phoenix-core/src/it/java/org/apache/phoenix/end2end/HashJoinIT.java phoenix-core/src/main/java/org/apache/phoenix/compile/JoinCompiler.java phoenix-core/src/main/java/org/apache/phoenix/parse/SelectStatement.java phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/compile/SubqueryRewriter.java phoenix-core/src/main/java/org/apache/phoenix/compile/TraceQueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeRewriter.java phoenix-core/src/main/java/org/apache/phoenix/compile/OrderByCompiler.java phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java phoenix-core/src/main/java/org/apache/phoenix/coprocessor/BaseScannerRegionObserver.java phoenix-core/src/main/java/org/apache/phoenix/execute/ScanPlan.java phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java phoenix-core/src/main/java/org/apache/phoenix/parse/DeleteStatement.java phoenix-core/src/main/java/org/apache/phoenix/parse/FilterableStatement.java phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java phoenix-core/src/main/java/org/apache/phoenix/iterate/LimitingResultIterator.java phoenix-core/src/test/java/org/apache/phoenix/execute/LiteralResultIteratorPlanTest.java phoenix-core/src/main/antlr3/PhoenixSQL.g phoenix-core/src/main/java/org/apache/phoenix/parse/OffsetNode.java phoenix-core/src/main/java/org/apache/phoenix/execute/SortMergeJoinPlan.java phoenix-core/src/main/java/org/apache/phoenix/execute/AggregatePlan.java phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java phoenix-core/src/main/java/org/apache/phoenix/compile/StatementNormalizer.java phoenix-core/src/main/java/org/apache/phoenix/compile/ListJarsQueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/execute/DegenerateQueryPlan.java phoenix-core/src/main/java/org/apache/phoenix/join/HashJoinInfo.java phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java phoenix-core/src/it/java/org/apache/phoenix/end2end/ReadOnlyIT.java phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java phoenix-core/src/main/java/org/apache/phoenix/compile/PostDDLCompiler.java phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetResultIterator.java phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java phoenix-core/src/it/java/org/apache/phoenix/end2end/AutoCommitIT.java phoenix-core/src/main/java/org/apache/phoenix/iterate/ParallelIterators.java phoenix-core/src/test/java/org/apache/phoenix/query/ParallelIteratorsSplitTest.java phoenix-core/src/it/java/org/apache/phoenix/end2end/SortMergeJoinIT.java phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedAggregatingResultIterator.java phoenix-core/src/test/java/org/apache/phoenix/execute/UnnestArrayPlanTest.java
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12797577/PHOENIX-2722_v1_rebased.patch
        against master branch at commit 91800b703d9879574ac5b3ee6fcfe6dd73af2148.
        ATTACHMENT ID: 12797577

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 11 new or modified tests.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 javadoc. The javadoc tool appears to have generated 24 warning messages.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 lineLengths. The patch introduces the following lines longer than 100:
        + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable LIMIT 1 OFFSET 1 ) AS t WHERE t.b = '"
        + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 ) AS t OFFSET 2";
        + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable OFFSET 4) AS t WHERE t.b = '"
        + query = "SELECT t.c, count FROM (SELECT count c FROM aTable GROUP BY a_string) AS t GROUP BY t.c ORDER BY count DESC OFFSET 1";
        + String query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT 2 OFFSET 1) AS t";
        + query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT 2 OFFSET 1) AS t LIMIT 4 OFFSET 1";
        + query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT 4 OFFSET 1) AS t LIMIT 2";
        + query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT ? OFFSET ?) AS t LIMIT ? OFFSET ?";
        + query = "SELECT a, s FROM (SELECT a_string a, sum(a_byte) s FROM aTable GROUP BY a_string ORDER BY sum(a_byte) OFFSET 1)";
        + query = "SELECT a_string, count FROM (SELECT a_string FROM aTable where a_byte < 4 union all SELECT a_string FROM aTable where a_byte > 8 OFFSET 1) group by a_string";

        -1 core tests. The patch failed these unit tests:
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.hbase.index.covered.example.EndToEndCoveredIndexingIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DeleteIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.MutableIndexIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CountDistinctCompressionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ArithmeticQueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.VariableLengthPKIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryTimeoutIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SpillableGroupByIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UserDefinedFunctionsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexMetadataIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ConnectionUtilIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.MappingTableDataTypeIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ClientTimeArithmeticQueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.MultiCfQueryExecIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AlterTableIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CsvBulkLoadToolIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.NativeHBaseTypesIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.MutableIndexFailureIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ReadOnlyIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UnionAllIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexWithStatsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryWithLimitIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ParallelIteratorsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificTablesDMLIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.TxWriteFailureIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ArrayAppendFunctionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.NotQueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ContextClassloaderIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.hbase.index.covered.example.EndtoEndIndexingWithCompressionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.PointInTimeQueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.StatsCollectionDisabledIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ArrayIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TxCheckpointIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.MutableIndexToolIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TransactionalViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SaltedViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpsertValuesIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CastAndCoerceIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.KeyOnlyIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.iterate.ScannerLeaseRenewalIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexExpressionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.MutableRollbackIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AlterSessionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AlterTableWithViewsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.StatsCollectorIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.execute.PartialCommitIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.rpc.PhoenixClientRpcIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ColumnProjectionOptimizationIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.RollbackIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CaseStatementIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.StatsCollectorWithSplitsAndMultiCFIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.GroupByIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.iterate.RoundRobinResultIteratorWithStatsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ReadOnlyIndexFailureIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.rpc.PhoenixServerRpcIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CreateTableIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.coprocessor.StatisticsCollectionRunTrackerIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ExecuteStatementsIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TransactionIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificTablesDDLIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryDatabaseMetaDataIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryWithOffsetIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.LocalIndexIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ScanQueryIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.DropViewIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AbsFunctionEnd2EndIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.MutableIndexReplicationIT
        ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AggregateQueryIT

        Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/297//testReport/
        Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/297//artifact/patchprocess/patchJavadocWarnings.txt
        Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/297//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12797577/PHOENIX-2722_v1_rebased.patch against master branch at commit 91800b703d9879574ac5b3ee6fcfe6dd73af2148. ATTACHMENT ID: 12797577 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 11 new or modified tests. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 javadoc . The javadoc tool appears to have generated 24 warning messages. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 lineLengths . The patch introduces the following lines longer than 100: + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable LIMIT 1 OFFSET 1 ) AS t WHERE t.b = '" + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 ) AS t OFFSET 2"; + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable OFFSET 4) AS t WHERE t.b = '" + query = "SELECT t.c, count FROM (SELECT count c FROM aTable GROUP BY a_string) AS t GROUP BY t.c ORDER BY count DESC OFFSET 1"; + String query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT 2 OFFSET 1) AS t"; + query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT 2 OFFSET 1) AS t LIMIT 4 OFFSET 1"; + query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT 4 OFFSET 1) AS t LIMIT 2"; + query = "SELECT t.eid FROM (SELECT entity_id eid FROM aTable LIMIT ? OFFSET ?) AS t LIMIT ? OFFSET ?"; + query = "SELECT a, s FROM (SELECT a_string a, sum(a_byte) s FROM aTable GROUP BY a_string ORDER BY sum(a_byte) OFFSET 1)"; + query = "SELECT a_string, count FROM (SELECT a_string FROM aTable where a_byte < 4 union all SELECT a_string FROM aTable where a_byte > 8 OFFSET 1) group by a_string"; -1 core tests . The patch failed these unit tests: ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.hbase.index.covered.example.EndToEndCoveredIndexingIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.DeleteIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.MutableIndexIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CountDistinctCompressionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ArithmeticQueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.VariableLengthPKIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryTimeoutIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SpillableGroupByIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UserDefinedFunctionsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexMetadataIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ConnectionUtilIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.MappingTableDataTypeIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ClientTimeArithmeticQueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.MultiCfQueryExecIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AlterTableIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CsvBulkLoadToolIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.NativeHBaseTypesIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.MutableIndexFailureIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ReadOnlyIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UnionAllIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexWithStatsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryWithLimitIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ParallelIteratorsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificTablesDMLIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.TxWriteFailureIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ArrayAppendFunctionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.NotQueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ContextClassloaderIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.hbase.index.covered.example.EndtoEndIndexingWithCompressionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.PointInTimeQueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.StatsCollectionDisabledIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ArrayIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TxCheckpointIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.monitoring.PhoenixMetricsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.MutableIndexToolIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TransactionalViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.SaltedViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.UpsertValuesIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CastAndCoerceIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.KeyOnlyIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.iterate.ScannerLeaseRenewalIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexExpressionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.MutableRollbackIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AlterSessionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AlterTableWithViewsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.StatsCollectorIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.execute.PartialCommitIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.IndexIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.rpc.PhoenixClientRpcIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ColumnProjectionOptimizationIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.txn.RollbackIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CaseStatementIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.StatsCollectorWithSplitsAndMultiCFIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.GroupByIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.iterate.RoundRobinResultIteratorWithStatsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ReadOnlyIndexFailureIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.rpc.PhoenixServerRpcIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.CreateTableIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.coprocessor.StatisticsCollectionRunTrackerIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ExecuteStatementsIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.tx.TransactionIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.TenantSpecificTablesDDLIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryDatabaseMetaDataIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.QueryWithOffsetIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.IndexToolIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.LocalIndexIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.ImmutableIndexIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.ScanQueryIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.DropViewIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AbsFunctionEnd2EndIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.index.MutableIndexReplicationIT ./phoenix-core/target/failsafe-reports/TEST-org.apache.phoenix.end2end.AggregateQueryIT Test results: https://builds.apache.org/job/PreCommit-PHOENIX-Build/297//testReport/ Javadoc warnings: https://builds.apache.org/job/PreCommit-PHOENIX-Build/297//artifact/patchprocess/patchJavadocWarnings.txt Console output: https://builds.apache.org/job/PreCommit-PHOENIX-Build/297//console This message is automatically generated.
        Hide
        jamestaylor James Taylor added a comment -

        +1. Thanks for the excellent work, Ankit Singhal and for diligently working through all the review comments.

        Show
        jamestaylor James Taylor added a comment - +1. Thanks for the excellent work, Ankit Singhal and for diligently working through all the review comments.
        Hide
        ankit.singhal Ankit Singhal added a comment -

        James Taylor, is it ready for commit now?

        Show
        ankit.singhal Ankit Singhal added a comment - James Taylor , is it ready for commit now?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58678464

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java —
        @@ -0,0 +1,44 @@
        +/*
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +package org.apache.phoenix.iterate;
        +
        +import java.util.List;
        +
        +import org.apache.hadoop.hbase.client.Scan;
        +import org.apache.phoenix.compile.QueryPlan;
        +
        +/**
        + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort),
        + * or if a scan crosses a region boundary and the table is salted or a local index.
        + */
        +public class OffsetScanGrouper implements ParallelScanGrouper {
        — End diff –

        It seems for serialIterators also , we use DefaultParallelScanGrouper for splitting scans in groups by condition `(isSalted || table.getIndexType() == IndexType.LOCAL) && ScanUtil.shouldRowsBeInRowKeyOrder(orderBy, context)` and run each group in one thread.

        Anyways, I have modified the plan to run offset on server by including this condition and re-use the DefaultParallelScanGrouper

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58678464 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java — @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import java.util.List; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.phoenix.compile.QueryPlan; + +/** + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort), + * or if a scan crosses a region boundary and the table is salted or a local index. + */ +public class OffsetScanGrouper implements ParallelScanGrouper { — End diff – It seems for serialIterators also , we use DefaultParallelScanGrouper for splitting scans in groups by condition `(isSalted || table.getIndexType() == IndexType.LOCAL) && ScanUtil.shouldRowsBeInRowKeyOrder(orderBy, context)` and run each group in one thread. Anyways, I have modified the plan to run offset on server by including this condition and re-use the DefaultParallelScanGrouper
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58661601

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java —
        @@ -0,0 +1,44 @@
        +/*
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +package org.apache.phoenix.iterate;
        +
        +import java.util.List;
        +
        +import org.apache.hadoop.hbase.client.Scan;
        +import org.apache.phoenix.compile.QueryPlan;
        +
        +/**
        + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort),
        + * or if a scan crosses a region boundary and the table is salted or a local index.
        + */
        +public class OffsetScanGrouper implements ParallelScanGrouper {
        — End diff –

        Scans aren't done in parallel for SerialIterator, they're done serially. Have you tried it without this? I can't figure out what purpose it's serving, but perhaps I'm missing something?

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58661601 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java — @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import java.util.List; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.phoenix.compile.QueryPlan; + +/** + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort), + * or if a scan crosses a region boundary and the table is salted or a local index. + */ +public class OffsetScanGrouper implements ParallelScanGrouper { — End diff – Scans aren't done in parallel for SerialIterator, they're done serially. Have you tried it without this? I can't figure out what purpose it's serving, but perhaps I'm missing something?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58646326

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java —
        @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec
        }
        }

        + OffsetNode offset = select.getOffset();
        + if (offsetRewrite != null || (limitRewrite != null & offset != null)) {
        — End diff –

        You're right, @ankitsinghal - won't have an impact.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58646326 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java — @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec } } + OffsetNode offset = select.getOffset(); + if (offsetRewrite != null || (limitRewrite != null & offset != null)) { — End diff – You're right, @ankitsinghal - won't have an impact.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58591817

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java —
        @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec
        }
        }

        + OffsetNode offset = select.getOffset();
        + if (offsetRewrite != null || (limitRewrite != null & offset != null)) {
        — End diff –

        @JamesRTaylor , will it really affect the boolean operands.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58591817 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java — @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec } } + OffsetNode offset = select.getOffset(); + if (offsetRewrite != null || (limitRewrite != null & offset != null)) { — End diff – @JamesRTaylor , will it really affect the boolean operands.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58588402

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java —
        @@ -0,0 +1,44 @@
        +/*
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +package org.apache.phoenix.iterate;
        +
        +import java.util.List;
        +
        +import org.apache.hadoop.hbase.client.Scan;
        +import org.apache.phoenix.compile.QueryPlan;
        +
        +/**
        + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort),
        + * or if a scan crosses a region boundary and the table is salted or a local index.
        + */
        +public class OffsetScanGrouper implements ParallelScanGrouper {
        — End diff –

        It is needed to avoid scans in parallel even in serialIterators. should I use a anonymous class while creating SerialIterators to do offset on server?

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58588402 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java — @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import java.util.List; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.phoenix.compile.QueryPlan; + +/** + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort), + * or if a scan crosses a region boundary and the table is salted or a local index. + */ +public class OffsetScanGrouper implements ParallelScanGrouper { — End diff – It is needed to avoid scans in parallel even in serialIterators. should I use a anonymous class while creating SerialIterators to do offset on server?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58558564

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java —
        @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec
        }
        }

        + OffsetNode offset = select.getOffset();
        + if (offsetRewrite != null || (limitRewrite != null & offset != null)) {
        — End diff –

        Based on this not being caught by one of your new unit tests, I'd say that there's likely a hole in the test coverage. Would you mind adding a unit test that would have caught this?

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58558564 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java — @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec } } + OffsetNode offset = select.getOffset(); + if (offsetRewrite != null || (limitRewrite != null & offset != null)) { — End diff – Based on this not being caught by one of your new unit tests, I'd say that there's likely a hole in the test coverage. Would you mind adding a unit test that would have caught this?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-205850495

        A few minor items, but otherwise it looks good. Nice work, @ankitsinghal! If you could file a JIRA for the corresponding website/doc changes and submit a patch for that when we're closer to the 4.8 release, that'd be much appreciated!

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-205850495 A few minor items, but otherwise it looks good. Nice work, @ankitsinghal! If you could file a JIRA for the corresponding website/doc changes and submit a patch for that when we're closer to the 4.8 release, that'd be much appreciated!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58555090

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java —
        @@ -138,6 +138,10 @@
        public final static String SYSTEM_SCHEMA_NAME = "SYSTEM";
        public final static byte[] SYSTEM_SCHEMA_NAME_BYTES = Bytes.toBytes(SYSTEM_SCHEMA_NAME);
        public final static String PHOENIX_METADATA = "table";
        + public final static String SCAN_OFFSET = "OFFSET";
        — End diff –

        Please change this to "_RowOffset" (to make is a little less general) and move to BaseScannerRegionObserver (this is where we keep all attributes we set on the Scan so we hopefully notice if we have a duplicate).

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58555090 — Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java — @@ -138,6 +138,10 @@ public final static String SYSTEM_SCHEMA_NAME = "SYSTEM"; public final static byte[] SYSTEM_SCHEMA_NAME_BYTES = Bytes.toBytes(SYSTEM_SCHEMA_NAME); public final static String PHOENIX_METADATA = "table"; + public final static String SCAN_OFFSET = "OFFSET"; — End diff – Please change this to "_RowOffset" (to make is a little less general) and move to BaseScannerRegionObserver (this is where we keep all attributes we set on the Scan so we hopefully notice if we have a duplicate).
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58553769

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java —
        @@ -130,9 +135,9 @@ public OrderedResultIterator(ResultIterator delegate, List<OrderByExpression> or
        SizedUtil.OBJECT_SIZE + estimatedRowSize;

        // Make sure we don't overflow Long, though this is really unlikely to happen.

        • assert(limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit);
          + assert (limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit + this.offset);
            • End diff –

        Based on the above diff, you added a space after "assert". Please remove so our formatting is consistent.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58553769 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java — @@ -130,9 +135,9 @@ public OrderedResultIterator(ResultIterator delegate, List<OrderByExpression> or SizedUtil.OBJECT_SIZE + estimatedRowSize; // Make sure we don't overflow Long, though this is really unlikely to happen. assert(limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit); + assert (limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit + this.offset); End diff – Based on the above diff, you added a space after "assert". Please remove so our formatting is consistent.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58553617

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java —
        @@ -0,0 +1,44 @@
        +/*
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +package org.apache.phoenix.iterate;
        +
        +import java.util.List;
        +
        +import org.apache.hadoop.hbase.client.Scan;
        +import org.apache.phoenix.compile.QueryPlan;
        +
        +/**
        + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort),
        + * or if a scan crosses a region boundary and the table is salted or a local index.
        + */
        +public class OffsetScanGrouper implements ParallelScanGrouper {
        — End diff –

        Why is this class needed as it doesn't appear to do anything.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58553617 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OffsetScanGrouper.java — @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import java.util.List; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.phoenix.compile.QueryPlan; + +/** + * Default implementation that creates a scan group if a plan is row key ordered (which requires a merge sort), + * or if a scan crosses a region boundary and the table is salted or a local index. + */ +public class OffsetScanGrouper implements ParallelScanGrouper { — End diff – Why is this class needed as it doesn't appear to do anything.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-205768420

        Thanks @JamesRTaylor for review.. I have made the changes are per your review comments.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-205768420 Thanks @JamesRTaylor for review.. I have made the changes are per your review comments.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58523285

        — Diff: phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java —
        @@ -172,17 +172,38 @@ public void testCorrelatePlanWithSingleValueOnly() throws SQLException {

        {2, "2", "2", 20}

        ,

        {5, "5", "5", 100}

        ,
        };

        • testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected);
          + testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected);
          }
        • private void testCorrelatePlan(Object[][] leftRelation, Object[][] rightRelation, int leftCorrelColumn, int rightCorrelColumn, JoinType type, Object[][] expectedResult) throws SQLException {
          + @Test
            • End diff –

        Added in a last commit.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58523285 — Diff: phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java — @@ -172,17 +172,38 @@ public void testCorrelatePlanWithSingleValueOnly() throws SQLException { {2, "2", "2", 20} , {5, "5", "5", 100} , }; testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected); + testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected); } private void testCorrelatePlan(Object[][] leftRelation, Object[][] rightRelation, int leftCorrelColumn, int rightCorrelColumn, JoinType type, Object[][] expectedResult) throws SQLException { + @Test End diff – Added in a last commit.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58523267

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java —
        @@ -130,9 +135,9 @@ public OrderedResultIterator(ResultIterator delegate, List<OrderByExpression> or
        SizedUtil.OBJECT_SIZE + estimatedRowSize;

        // Make sure we don't overflow Long, though this is really unlikely to happen.

        • assert(limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit);
          + assert (limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit + this.offset);
            • End diff –

        I checked and there was no space before assert.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58523267 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java — @@ -130,9 +135,9 @@ public OrderedResultIterator(ResultIterator delegate, List<OrderByExpression> or SizedUtil.OBJECT_SIZE + estimatedRowSize; // Make sure we don't overflow Long, though this is really unlikely to happen. assert(limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit); + assert (limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit + this.offset); End diff – I checked and there was no space before assert.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58512767

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSerialIterators.java —
        @@ -0,0 +1,129 @@
        +/*
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +package org.apache.phoenix.iterate;
        +
        +import static org.apache.phoenix.monitoring.MetricType.SCAN_BYTES;
        +
        +import java.sql.SQLException;
        +import java.util.Collections;
        +import java.util.List;
        +import java.util.Queue;
        +import java.util.concurrent.ExecutorService;
        +import java.util.concurrent.Future;
        +
        +import org.apache.hadoop.hbase.client.Scan;
        +import org.apache.hadoop.hbase.util.Bytes;
        +import org.apache.hadoop.hbase.util.Pair;
        +import org.apache.phoenix.compile.QueryPlan;
        +import org.apache.phoenix.jdbc.PhoenixConnection;
        +import org.apache.phoenix.job.JobManager.JobCallable;
        +import org.apache.phoenix.monitoring.TaskExecutionMetricsHolder;
        +import org.apache.phoenix.query.QueryConstants;
        +import org.apache.phoenix.trace.util.Tracing;
        +import org.apache.phoenix.util.ScanUtil;
        +
        +import com.google.common.base.Preconditions;
        +import com.google.common.collect.Lists;
        +
        +
        +/**
        + *
        + * Class that parallelizes the scan over a table using the ExecutorService provided. Each region of the table will be scanned in parallel with
        + * the results accessible through

        {@link #getIterators()}

        + *
        + *
        + * @since 0.1
        + */
        +public class TableSerialIterators extends BaseResultIterators {
        — End diff –

        Yes, I am able re-used it now.. earlier I was facing some issues..

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58512767 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSerialIterators.java — @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import static org.apache.phoenix.monitoring.MetricType.SCAN_BYTES; + +import java.sql.SQLException; +import java.util.Collections; +import java.util.List; +import java.util.Queue; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.Pair; +import org.apache.phoenix.compile.QueryPlan; +import org.apache.phoenix.jdbc.PhoenixConnection; +import org.apache.phoenix.job.JobManager.JobCallable; +import org.apache.phoenix.monitoring.TaskExecutionMetricsHolder; +import org.apache.phoenix.query.QueryConstants; +import org.apache.phoenix.trace.util.Tracing; +import org.apache.phoenix.util.ScanUtil; + +import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; + + +/** + * + * Class that parallelizes the scan over a table using the ExecutorService provided. Each region of the table will be scanned in parallel with + * the results accessible through {@link #getIterators()} + * + * + * @since 0.1 + */ +public class TableSerialIterators extends BaseResultIterators { — End diff – Yes, I am able re-used it now.. earlier I was facing some issues..
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58506510

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java —
        @@ -79,9 +86,11 @@ public Tuple peek() throws SQLException {

        @Override
        public Tuple next() throws SQLException {

        • if (limit >= 0 && count++ >= limit) {
        • return null;
          + while (count < offset) {
          + if (super.next() == null) { return null; }
          + count++;
          }
          + if (limit >= 0 && count++ >= limit) { return null; }
            • End diff –

        Actually it should count++ only, but there is a bug in offset count .. which I have updated now.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58506510 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java — @@ -79,9 +86,11 @@ public Tuple peek() throws SQLException { @Override public Tuple next() throws SQLException { if (limit >= 0 && count++ >= limit) { return null; + while (count < offset) { + if (super.next() == null) { return null; } + count++; } + if (limit >= 0 && count++ >= limit) { return null; } End diff – Actually it should count++ only, but there is a bug in offset count .. which I have updated now.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58504753

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException {
        SelectStatement subSelect = unionAllSelects.get;
        // Push down order-by and limit into sub-selects.
        if (!select.getOrderBy().isEmpty() || select.getLimit() != null) {
        — End diff –

        I think the change is not required as per above comment.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58504753 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException { SelectStatement subSelect = unionAllSelects.get ; // Push down order-by and limit into sub-selects. if (!select.getOrderBy().isEmpty() || select.getLimit() != null) { — End diff – I think the change is not required as per above comment.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58504679

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException {
        SelectStatement subSelect = unionAllSelects.get;
        // Push down order-by and limit into sub-selects.
        if (!select.getOrderBy().isEmpty() || select.getLimit() != null) {

        • subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit());
          + if (select.getOffset() == null) { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null); + }

          else

          { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null); + }
            • End diff –

        Actually , In case of union , we need to apply offset at the final result so we can't apply limit or offset in subselect.
        but, if offset is not present , limit can be applied

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58504679 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException { SelectStatement subSelect = unionAllSelects.get ; // Push down order-by and limit into sub-selects. if (!select.getOrderBy().isEmpty() || select.getLimit() != null) { subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit()); + if (select.getOffset() == null) { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null); + } else { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null); + } End diff – Actually , In case of union , we need to apply offset at the final result so we can't apply limit or offset in subselect. but, if offset is not present , limit can be applied
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58504649

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException {
        SelectStatement subSelect = unionAllSelects.get;
        // Push down order-by and limit into sub-selects.
        if (!select.getOrderBy().isEmpty() || select.getLimit() != null) {

        • subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit());
          + if (select.getOffset() == null) { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null); + }

          else

          { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null); + }
            • End diff –

        Actually , In case of union , we need to apply offset at the final result so we can't apply limit or offset in subselect.
        but, if offset is not present , limit can be applied

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58504649 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException { SelectStatement subSelect = unionAllSelects.get ; // Push down order-by and limit into sub-selects. if (!select.getOrderBy().isEmpty() || select.getLimit() != null) { subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit()); + if (select.getOffset() == null) { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null); + } else { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null); + } End diff – Actually , In case of union , we need to apply offset at the final result so we can't apply limit or offset in subselect. but, if offset is not present , limit can be applied
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58470085

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSerialIterators.java —
        @@ -0,0 +1,129 @@
        +/*
        + * Licensed to the Apache Software Foundation (ASF) under one
        + * or more contributor license agreements. See the NOTICE file
        + * distributed with this work for additional information
        + * regarding copyright ownership. The ASF licenses this file
        + * to you under the Apache License, Version 2.0 (the
        + * "License"); you may not use this file except in compliance
        + * with the License. You may obtain a copy of the License at
        + *
        + * http://www.apache.org/licenses/LICENSE-2.0
        + *
        + * Unless required by applicable law or agreed to in writing, software
        + * distributed under the License is distributed on an "AS IS" BASIS,
        + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
        + * See the License for the specific language governing permissions and
        + * limitations under the License.
        + */
        +package org.apache.phoenix.iterate;
        +
        +import static org.apache.phoenix.monitoring.MetricType.SCAN_BYTES;
        +
        +import java.sql.SQLException;
        +import java.util.Collections;
        +import java.util.List;
        +import java.util.Queue;
        +import java.util.concurrent.ExecutorService;
        +import java.util.concurrent.Future;
        +
        +import org.apache.hadoop.hbase.client.Scan;
        +import org.apache.hadoop.hbase.util.Bytes;
        +import org.apache.hadoop.hbase.util.Pair;
        +import org.apache.phoenix.compile.QueryPlan;
        +import org.apache.phoenix.jdbc.PhoenixConnection;
        +import org.apache.phoenix.job.JobManager.JobCallable;
        +import org.apache.phoenix.monitoring.TaskExecutionMetricsHolder;
        +import org.apache.phoenix.query.QueryConstants;
        +import org.apache.phoenix.trace.util.Tracing;
        +import org.apache.phoenix.util.ScanUtil;
        +
        +import com.google.common.base.Preconditions;
        +import com.google.common.collect.Lists;
        +
        +
        +/**
        + *
        + * Class that parallelizes the scan over a table using the ExecutorService provided. Each region of the table will be scanned in parallel with
        + * the results accessible through

        {@link #getIterators()}

        + *
        + *
        + * @since 0.1
        + */
        +public class TableSerialIterators extends BaseResultIterators {
        — End diff –

        We have SerialIterators already. Any reason why we can't use that? I'd hate to introduce a new one if it's not absolutely necessary.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58470085 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSerialIterators.java — @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.iterate; + +import static org.apache.phoenix.monitoring.MetricType.SCAN_BYTES; + +import java.sql.SQLException; +import java.util.Collections; +import java.util.List; +import java.util.Queue; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; + +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.Pair; +import org.apache.phoenix.compile.QueryPlan; +import org.apache.phoenix.jdbc.PhoenixConnection; +import org.apache.phoenix.job.JobManager.JobCallable; +import org.apache.phoenix.monitoring.TaskExecutionMetricsHolder; +import org.apache.phoenix.query.QueryConstants; +import org.apache.phoenix.trace.util.Tracing; +import org.apache.phoenix.util.ScanUtil; + +import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; + + +/** + * + * Class that parallelizes the scan over a table using the ExecutorService provided. Each region of the table will be scanned in parallel with + * the results accessible through {@link #getIterators()} + * + * + * @since 0.1 + */ +public class TableSerialIterators extends BaseResultIterators { — End diff – We have SerialIterators already. Any reason why we can't use that? I'd hate to introduce a new one if it's not absolutely necessary.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58469780

        — Diff: phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java —
        @@ -172,17 +172,38 @@ public void testCorrelatePlanWithSingleValueOnly() throws SQLException {

        {2, "2", "2", 20}

        ,

        {5, "5", "5", 100}

        ,
        };

        • testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected);
          + testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected);
          }
        • private void testCorrelatePlan(Object[][] leftRelation, Object[][] rightRelation, int leftCorrelColumn, int rightCorrelColumn, JoinType type, Object[][] expectedResult) throws SQLException {
          + @Test
            • End diff –

        Also, test for bind parameter for OFFSET, like this:

        SELECT * FROM T OFFSET ?

        And check that the ResultSetMetaData and ParameterMetaData are ok.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58469780 — Diff: phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java — @@ -172,17 +172,38 @@ public void testCorrelatePlanWithSingleValueOnly() throws SQLException { {2, "2", "2", 20} , {5, "5", "5", 100} , }; testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected); + testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected); } private void testCorrelatePlan(Object[][] leftRelation, Object[][] rightRelation, int leftCorrelColumn, int rightCorrelColumn, JoinType type, Object[][] expectedResult) throws SQLException { + @Test End diff – Also, test for bind parameter for OFFSET, like this: SELECT * FROM T OFFSET ? And check that the ResultSetMetaData and ParameterMetaData are ok.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58469617

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java —
        @@ -138,6 +138,9 @@
        public final static String SYSTEM_SCHEMA_NAME = "SYSTEM";
        public final static byte[] SYSTEM_SCHEMA_NAME_BYTES = Bytes.toBytes(SYSTEM_SCHEMA_NAME);
        public final static String PHOENIX_METADATA = "table";
        + public final static String OFFSET = "offset";
        + public final static byte[] offsetRowKeyBytes = "OFFSET".getBytes();
        — End diff –

        Nit: use Bytes.toBytes(OFFSET) instead. Also, did you want it to be "OFFSET" for the OFFSET constant?

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58469617 — Diff: phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java — @@ -138,6 +138,9 @@ public final static String SYSTEM_SCHEMA_NAME = "SYSTEM"; public final static byte[] SYSTEM_SCHEMA_NAME_BYTES = Bytes.toBytes(SYSTEM_SCHEMA_NAME); public final static String PHOENIX_METADATA = "table"; + public final static String OFFSET = "offset"; + public final static byte[] offsetRowKeyBytes = " OFFSET ".getBytes(); — End diff – Nit: use Bytes.toBytes(OFFSET) instead. Also, did you want it to be " OFFSET " for the OFFSET constant?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58469287

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java —
        @@ -130,9 +135,9 @@ public OrderedResultIterator(ResultIterator delegate, List<OrderByExpression> or
        SizedUtil.OBJECT_SIZE + estimatedRowSize;

        // Make sure we don't overflow Long, though this is really unlikely to happen.

        • assert(limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit);
          + assert (limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit + this.offset);
            • End diff –

        Nit: remove space before assert

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58469287 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/OrderedResultIterator.java — @@ -130,9 +135,9 @@ public OrderedResultIterator(ResultIterator delegate, List<OrderByExpression> or SizedUtil.OBJECT_SIZE + estimatedRowSize; // Make sure we don't overflow Long, though this is really unlikely to happen. assert(limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit); + assert (limit == null || Long.MAX_VALUE / estimatedEntrySize >= limit + this.offset); End diff – Nit: remove space before assert
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58469109

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java —
        @@ -79,9 +86,11 @@ public Tuple peek() throws SQLException {

        @Override
        public Tuple next() throws SQLException {

        • if (limit >= 0 && count++ >= limit) {
        • return null;
          + while (count < offset) {
          + if (super.next() == null) { return null; }
          + count++;
          }
          + if (limit >= 0 && count++ >= limit) { return null; }
            • End diff –

        Should this be just count or count++ ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58469109 — Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/MergeSortTopNResultIterator.java — @@ -79,9 +86,11 @@ public Tuple peek() throws SQLException { @Override public Tuple next() throws SQLException { if (limit >= 0 && count++ >= limit) { return null; + while (count < offset) { + if (super.next() == null) { return null; } + count++; } + if (limit >= 0 && count++ >= limit) { return null; } End diff – Should this be just count or count++ ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58467402

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java —
        @@ -218,7 +228,11 @@ protected RegionScanner doPostScannerOpen(final ObserverContext<RegionCoprocesso
        if (j != null)

        { innerScanner = new HashJoinRegionScanner(innerScanner, p, j, tenantId, c.getEnvironment()); }

        -
        + if (scanOffset != null) {
        + innerScanner = getOffsetScanner(c, innerScanner,
        + new OffsetResultIterator(new RegionScannerResultIterator(innerScanner), scanOffset),
        + scan.getAttribute(QueryConstants.LAST_SCAN) == null ? false : true);
        — End diff –

        Nit: change this:

        scan.getAttribute(QueryConstants.LAST_SCAN) == null ? false : true

        to this:

        scan.getAttribute(QueryConstants.LAST_SCAN) != null

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58467402 — Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java — @@ -218,7 +228,11 @@ protected RegionScanner doPostScannerOpen(final ObserverContext<RegionCoprocesso if (j != null) { innerScanner = new HashJoinRegionScanner(innerScanner, p, j, tenantId, c.getEnvironment()); } - + if (scanOffset != null) { + innerScanner = getOffsetScanner(c, innerScanner, + new OffsetResultIterator(new RegionScannerResultIterator(innerScanner), scanOffset), + scan.getAttribute(QueryConstants.LAST_SCAN) == null ? false : true); — End diff – Nit: change this: scan.getAttribute(QueryConstants.LAST_SCAN) == null ? false : true to this: scan.getAttribute(QueryConstants.LAST_SCAN) != null
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58467131

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java —
        @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec
        }
        }

        + OffsetNode offset = select.getOffset();
        + if (offsetRewrite != null || (limitRewrite != null & offset != null)) {
        — End diff –

        Single & operator here? Shouldn't it be && ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58467131 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java — @@ -188,6 +190,13 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec } } + OffsetNode offset = select.getOffset(); + if (offsetRewrite != null || (limitRewrite != null & offset != null)) { — End diff – Single & operator here? Shouldn't it be && ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58467053

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubqueryRewriter.java —
        @@ -167,7 +167,7 @@ public ParseNode visitLeave(ExistsParseNode node, List<ParseNode> l) throws SQLE
        JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, rhsTableAlias);
        ParseNode where = subquery.getWhere() == null ? null : subquery.getWhere().accept(conditionExtractor);
        if (where == subquery.getWhere()) { // non-correlated EXISTS subquery, add LIMIT 1

        • subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(1)));
          + subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(1)), null);
            • End diff –

        Always null here? Just want to confirm that this is correct.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58467053 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubqueryRewriter.java — @@ -167,7 +167,7 @@ public ParseNode visitLeave(ExistsParseNode node, List<ParseNode> l) throws SQLE JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, rhsTableAlias); ParseNode where = subquery.getWhere() == null ? null : subquery.getWhere().accept(conditionExtractor); if (where == subquery.getWhere()) { // non-correlated EXISTS subquery, add LIMIT 1 subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(1))); + subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(1)), null); End diff – Always null here? Just want to confirm that this is correct.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58466849

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException {
        SelectStatement subSelect = unionAllSelects.get;
        // Push down order-by and limit into sub-selects.
        if (!select.getOrderBy().isEmpty() || select.getLimit() != null) {
        — End diff –

        Does this if statement need to be change to this (and if so, check for other occurrences)?

        if (!select.getOrderBy().isEmpty() || select.getLimit() != null || select.getOffset() != null)

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58466849 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException { SelectStatement subSelect = unionAllSelects.get ; // Push down order-by and limit into sub-selects. if (!select.getOrderBy().isEmpty() || select.getLimit() != null) { — End diff – Does this if statement need to be change to this (and if so, check for other occurrences)? if (!select.getOrderBy().isEmpty() || select.getLimit() != null || select.getOffset() != null)
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58466741

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException {
        SelectStatement subSelect = unionAllSelects.get;
        // Push down order-by and limit into sub-selects.
        if (!select.getOrderBy().isEmpty() || select.getLimit() != null) {

        • subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit());
          + if (select.getOffset() == null) { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null); + }

          else

          { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null); + }
            • End diff –

        Rather than this if statement, can we do the following to simplify it?

        subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), select.getOffset())

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58466741 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -168,7 +169,11 @@ public QueryPlan compileUnionAll(SelectStatement select) throws SQLException { SelectStatement subSelect = unionAllSelects.get ; // Push down order-by and limit into sub-selects. if (!select.getOrderBy().isEmpty() || select.getLimit() != null) { subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit()); + if (select.getOffset() == null) { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), null); + } else { + subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), null, null); + } End diff – Rather than this if statement, can we do the following to simplify it? subSelect = NODE_FACTORY.select(subSelect, select.getOrderBy(), select.getLimit(), select.getOffset())
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-205461513

        Hi @ankitsinghal, sorry about the delay. I don't get github notification in mailbox... It looks good to me now. Thank you for all the work! @JamesRTaylor What do you think?

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-205461513 Hi @ankitsinghal, sorry about the delay. I don't get github notification in mailbox... It looks good to me now. Thank you for all the work! @JamesRTaylor What do you think?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-205446525

        @maryannxue , what do you think .. is it good to go now?

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-205446525 @maryannxue , what do you think .. is it good to go now?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-204083599

        I have taken care the last review comments in my latest commit.
        Let me know if it is good to know. Thanks you so much @maryannxue for review.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-204083599 I have taken care the last review comments in my latest commit. Let me know if it is good to know. Thanks you so much @maryannxue for review.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r58109985

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java —
        @@ -85,7 +85,8 @@ public void close() throws SQLException {

        @Override
        public Tuple next() throws SQLException {

        • while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext()) {
          + while (!this.closed && (offset != null && count < offset) && tupleIterator.hasNext()) {
          + count++;
            • End diff –

        Ah, I was in a notion that I updated limit to limit+offset already.
        Thanks for catching it, I have corrected it now.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r58109985 — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java — @@ -85,7 +85,8 @@ public void close() throws SQLException { @Override public Tuple next() throws SQLException { while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext()) { + while (!this.closed && (offset != null && count < offset) && tupleIterator.hasNext()) { + count++; End diff – Ah, I was in a notion that I updated limit to limit+offset already. Thanks for catching it, I have corrected it now.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-203722854

        @ankitsinghal Some (maybe final) comments added.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-203722854 @ankitsinghal Some (maybe final) comments added.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57993968

        — Diff: phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java —
        @@ -172,17 +172,38 @@ public void testCorrelatePlanWithSingleValueOnly() throws SQLException {

        {2, "2", "2", 20}

        ,

        {5, "5", "5", 100}

        ,
        };

        • testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected);
          + testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected);
          }
        • private void testCorrelatePlan(Object[][] leftRelation, Object[][] rightRelation, int leftCorrelColumn, int rightCorrelColumn, JoinType type, Object[][] expectedResult) throws SQLException {
          + @Test
            • End diff –

        Maybe add an independent test file for LiteralResultIterationPlan test, testing OFFSET, LIMIT, and OFFSET + LIMIT.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57993968 — Diff: phoenix-core/src/test/java/org/apache/phoenix/execute/CorrelatePlanTest.java — @@ -172,17 +172,38 @@ public void testCorrelatePlanWithSingleValueOnly() throws SQLException { {2, "2", "2", 20} , {5, "5", "5", 100} , }; testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected); + testCorrelatePlan(LEFT_RELATION, rightRelation, 1, 0, JoinType.Inner, expected); } private void testCorrelatePlan(Object[][] leftRelation, Object[][] rightRelation, int leftCorrelColumn, int rightCorrelColumn, JoinType type, Object[][] expectedResult) throws SQLException { + @Test End diff – Maybe add an independent test file for LiteralResultIterationPlan test, testing OFFSET, LIMIT, and OFFSET + LIMIT.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57993856

        — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java —
        @@ -241,7 +241,7 @@ public void testDerivedTableWithWhere() throws Exception {
        rs = statement.executeQuery();
        assertFalse(rs.next());

        • // (offset) where
          + // (where offset)
            • End diff –

        Guess it doesn't make too much sense here to keep this (where offset) case. You can simply remove it, or otherwise, make it "(where) offset" which is essentially the same as (where offset) but is more meaningful for testing query flattening.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57993856 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java — @@ -241,7 +241,7 @@ public void testDerivedTableWithWhere() throws Exception { rs = statement.executeQuery(); assertFalse(rs.next()); // (offset) where + // (where offset) End diff – Guess it doesn't make too much sense here to keep this (where offset) case. You can simply remove it, or otherwise, make it "(where) offset" which is essentially the same as (where offset) but is more meaningful for testing query flattening.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57993753

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java —
        @@ -85,7 +85,8 @@ public void close() throws SQLException {

        @Override
        public Tuple next() throws SQLException {

        • while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext()) {
          + while (!this.closed && (offset != null && count < offset) && tupleIterator.hasNext()) {
          + count++;
            • End diff –

        What I meant was this "count++" doesn't seem to work with LIMIT(below) together. LIMIT should be number of rows returned starting from the OFFSET row, right?

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57993753 — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java — @@ -85,7 +85,8 @@ public void close() throws SQLException { @Override public Tuple next() throws SQLException { while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext()) { + while (!this.closed && (offset != null && count < offset) && tupleIterator.hasNext()) { + count++; End diff – What I meant was this "count++" doesn't seem to work with LIMIT(below) together. LIMIT should be number of rows returned starting from the OFFSET row, right?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-202806077

        Thanks @maryannxue for suggestions , I have incorporated the review comments.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-202806077 Thanks @maryannxue for suggestions , I have incorporated the review comments.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-202671194

        @ankitsinghal Thank you very much for making the suggested changes! Added a few more suggestions, most of them being minor.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-202671194 @ankitsinghal Thank you very much for making the suggested changes! Added a few more suggestions, most of them being minor.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57664500

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java —
        @@ -85,6 +85,9 @@ public void close() throws SQLException {

        @Override
        public Tuple next() throws SQLException {
        + while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext())

        { + tupleIterator.next(); + }

        — End diff –

        This doesn't look right to me. Mind if you correct this and add a correponding test case?

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57664500 — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/LiteralResultIterationPlan.java — @@ -85,6 +85,9 @@ public void close() throws SQLException { @Override public Tuple next() throws SQLException { + while (!this.closed && (offset != null && count++ < offset) && tupleIterator.hasNext()) { + tupleIterator.next(); + } — End diff – This doesn't look right to me. Mind if you correct this and add a correponding test case?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57664292

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/ScanPlan.java —
        @@ -193,21 +194,32 @@ protected ResultIterator newIterator(ParallelScanGrouper scanGrouper) throws SQL

        • limit is provided, run query serially.
          */
          boolean isOrdered = !orderBy.getOrderByExpressions().isEmpty();
        • boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, allowPageFilter);
          + boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, offset, allowPageFilter);
          Integer perScanLimit = !allowPageFilter || isOrdered ? null : limit;
          + if (perScanLimit != null) {
          + perScanLimit += (offset == null ? 0 : offset);
            • End diff –

        Can it use QueryUtil.getOffsetLimit() ?

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57664292 — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/ScanPlan.java — @@ -193,21 +194,32 @@ protected ResultIterator newIterator(ParallelScanGrouper scanGrouper) throws SQL limit is provided, run query serially. */ boolean isOrdered = !orderBy.getOrderByExpressions().isEmpty(); boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, allowPageFilter); + boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, offset, allowPageFilter); Integer perScanLimit = !allowPageFilter || isOrdered ? null : limit; + if (perScanLimit != null) { + perScanLimit += (offset == null ? 0 : offset); End diff – Can it use QueryUtil.getOffsetLimit() ?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57664171

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/ScanPlan.java —
        @@ -193,21 +194,32 @@ protected ResultIterator newIterator(ParallelScanGrouper scanGrouper) throws SQL

        • limit is provided, run query serially.
          */
          boolean isOrdered = !orderBy.getOrderByExpressions().isEmpty();
        • boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, allowPageFilter);
          + boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, offset, allowPageFilter);
          Integer perScanLimit = !allowPageFilter || isOrdered ? null : limit;
          + if (perScanLimit != null) { + perScanLimit += (offset == null ? 0 : offset); + }

          + boolean hasOffset = offset != null;

            • End diff –

        Looks like there's <tab> here and a few lines below.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57664171 — Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/ScanPlan.java — @@ -193,21 +194,32 @@ protected ResultIterator newIterator(ParallelScanGrouper scanGrouper) throws SQL limit is provided, run query serially. */ boolean isOrdered = !orderBy.getOrderByExpressions().isEmpty(); boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, allowPageFilter); + boolean isSerial = isSerial(context, statement, tableRef, orderBy, limit, offset, allowPageFilter); Integer perScanLimit = !allowPageFilter || isOrdered ? null : limit; + if (perScanLimit != null) { + perScanLimit += (offset == null ? 0 : offset); + } + boolean hasOffset = offset != null; End diff – Looks like there's <tab> here and a few lines below.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57663883

        — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java —
        @@ -233,6 +233,34 @@ public void testDerivedTableWithWhere() throws Exception {
        assertEquals(9,rs.getInt(1));

        assertFalse(rs.next());
        +
        + // Inner limit < outer query offset
        + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable LIMIT 1 OFFSET 1 ) AS t WHERE t.b = '"
        + + C_VALUE + "' OFFSET 2";
        + statement = conn.prepareStatement(query);
        + rs = statement.executeQuery();
        + assertFalse(rs.next());
        +
        + // (offset) where
        + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 OFFSET 2) AS t";
        — End diff –

        This is (offset where) rather than (offset) where. I think it would make more sense to test "(offset) where" here.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57663883 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java — @@ -233,6 +233,34 @@ public void testDerivedTableWithWhere() throws Exception { assertEquals(9,rs.getInt(1)); assertFalse(rs.next()); + + // Inner limit < outer query offset + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable LIMIT 1 OFFSET 1 ) AS t WHERE t.b = '" + + C_VALUE + "' OFFSET 2"; + statement = conn.prepareStatement(query); + rs = statement.executeQuery(); + assertFalse(rs.next()); + + // (offset) where + query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 OFFSET 2) AS t"; — End diff – This is (offset where) rather than (offset) where. I think it would make more sense to test "(offset) where" here.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57663830

        — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryWithLimitIT.java —
        @@ -20,6 +20,7 @@
        package org.apache.phoenix.end2end;

        import static org.apache.phoenix.util.TestUtil.KEYONLY_NAME;
        +import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
        — End diff –

        unnecessary diff

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57663830 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryWithLimitIT.java — @@ -20,6 +20,7 @@ package org.apache.phoenix.end2end; import static org.apache.phoenix.util.TestUtil.KEYONLY_NAME; +import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES; — End diff – unnecessary diff
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57663136

        — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java —
        @@ -134,7 +134,7 @@ public void testDerivedTableWithWhere() throws Exception {
        Connection conn = DriverManager.getConnection(getUrl(), props);
        try {
        // (where)

        • String query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9) AS t";
          + String query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 ) AS t";
          PreparedStatement statement = conn.prepareStatement(query);
            • End diff –

        Unnecessary diff

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57663136 — Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java — @@ -134,7 +134,7 @@ public void testDerivedTableWithWhere() throws Exception { Connection conn = DriverManager.getConnection(getUrl(), props); try { // (where) String query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9) AS t"; + String query = "SELECT t.eid, t.x + 9 FROM (SELECT entity_id eid, b_string b, a_byte + 1 x FROM aTable WHERE a_byte + 1 < 9 ) AS t"; PreparedStatement statement = conn.prepareStatement(query); End diff – Unnecessary diff
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-202281988

        @maryannxue Thank you so much for the review! I have incorporated the changes as per your comments and added more test cases related to join, derived table ,group by case and join with subquery as you asked.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-202281988 @maryannxue Thank you so much for the review! I have incorporated the changes as per your comments and added more test cases related to join, derived table ,group by case and join with subquery as you asked.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57551123

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -324,10 +328,13 @@ protected QueryPlan compileJoinQuery(StatementContext context, List<Object> bind
        QueryPlan plan = compileSingleFlatQuery(context, query, binds, asSubquery, !asSubquery && joinTable.isAllLeftJoin(), null, !table.isSubselect() && projectPKColumns ? tupleProjector : null, true);
        Expression postJoinFilterExpression = joinTable.compilePostFilterExpression(context, table);
        Integer limit = null;
        + Integer offset = null;
        if (!query.isAggregate() && !query.isDistinct() && query.getOrderBy().isEmpty())

        { limit = plan.getLimit(); + offset = plan.getOffset(); }
        • HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes, starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit);
          + HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes,
          + starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit, offset);
            • End diff –

        updated in the latest commit

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57551123 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -324,10 +328,13 @@ protected QueryPlan compileJoinQuery(StatementContext context, List<Object> bind QueryPlan plan = compileSingleFlatQuery(context, query, binds, asSubquery, !asSubquery && joinTable.isAllLeftJoin(), null, !table.isSubselect() && projectPKColumns ? tupleProjector : null, true); Expression postJoinFilterExpression = joinTable.compilePostFilterExpression(context, table); Integer limit = null; + Integer offset = null; if (!query.isAggregate() && !query.isDistinct() && query.getOrderBy().isEmpty()) { limit = plan.getLimit(); + offset = plan.getOffset(); } HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes, starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit); + HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes, + starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit, offset); End diff – updated in the latest commit
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user ankitsinghal commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57551094

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java —
        @@ -202,14 +204,30 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec
        }
        }
        }

        • + OffsetNode offset = select.getOffset();
          + if (offset != null) {
          + if (offsetRewrite == null)

          { + offsetRewrite = offset; + }

          else

          Unknown macro: { + Integer offsetValue = OffsetCompiler.compile(null, select); + Integer offsetValueSubselect = OffsetCompiler.compile(null, subselect); + if (offsetValue != null && offsetValueSubselect != null) { + offsetRewrite = offsetValue < offsetValueSubselect ? offset : offsetRewrite; + } else { + return select; + } + }

          + }
          +

            • End diff –

        Thanks for pointing out. I thought a little and have updated the logic to
        if (offsetRewrite != null || (limitRewrite != null & offset != null))

        { return select; }

        else

        { offsetRewrite = offset; }

        Let me know if any optimization is possible.

        Show
        githubbot ASF GitHub Bot added a comment - Github user ankitsinghal commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57551094 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java — @@ -202,14 +204,30 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec } } } + OffsetNode offset = select.getOffset(); + if (offset != null) { + if (offsetRewrite == null) { + offsetRewrite = offset; + } else Unknown macro: { + Integer offsetValue = OffsetCompiler.compile(null, select); + Integer offsetValueSubselect = OffsetCompiler.compile(null, subselect); + if (offsetValue != null && offsetValueSubselect != null) { + offsetRewrite = offsetValue < offsetValueSubselect ? offset : offsetRewrite; + } else { + return select; + } + } + } + End diff – Thanks for pointing out. I thought a little and have updated the logic to if (offsetRewrite != null || (limitRewrite != null & offset != null)) { return select; } else { offsetRewrite = offset; } Let me know if any optimization is possible.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-200637757

        @ankitsinghal Thank you very much for the pull request! I am impressed by how many details you have taken into account in your commit. It took me quite a while to go through all of the changes. That said, it would be great if you could add more test cases covering most (if not all) of the code changes, e.g. the join case (left outer join w/ offset and w/wo limit), the derived table case, the join with subquery case, etc.

        Check JoinCompiler.isFlat(SelectStatement), it returns true only when limit == null, think it should be the same for offset. That function is called when a join contains a subquery, e.g. "select * from (select a, b from t1 offset 3) join (select c, d from t2 limit 10)".

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-200637757 @ankitsinghal Thank you very much for the pull request! I am impressed by how many details you have taken into account in your commit. It took me quite a while to go through all of the changes. That said, it would be great if you could add more test cases covering most (if not all) of the code changes, e.g. the join case (left outer join w/ offset and w/wo limit), the derived table case, the join with subquery case, etc. Check JoinCompiler.isFlat(SelectStatement), it returns true only when limit == null, think it should be the same for offset. That function is called when a join contains a subquery, e.g. "select * from (select a, b from t1 offset 3) join (select c, d from t2 limit 10)".
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57269833

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java —
        @@ -324,10 +328,13 @@ protected QueryPlan compileJoinQuery(StatementContext context, List<Object> bind
        QueryPlan plan = compileSingleFlatQuery(context, query, binds, asSubquery, !asSubquery && joinTable.isAllLeftJoin(), null, !table.isSubselect() && projectPKColumns ? tupleProjector : null, true);
        Expression postJoinFilterExpression = joinTable.compilePostFilterExpression(context, table);
        Integer limit = null;
        + Integer offset = null;
        if (!query.isAggregate() && !query.isDistinct() && query.getOrderBy().isEmpty())

        { limit = plan.getLimit(); + offset = plan.getOffset(); }
        • HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes, starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit);
          + HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes,
          + starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit, offset);
            • End diff –

        The "limit" in HashJoinInfo is a little different than the original SQL semantics. It specifies the max number of rows returned by a HashJoinRegionScanner so that it doesn't do too much unnecessary work, and its max rows per region, so in the end the client might get more rows than needed and will do a final limit operation (all done by ScanPlan, don't worry). So, here, I think the limit should be "offset + limit" and there is no need to add an extra "offset" field to HashJoinInfo. Btw, change in serialization/deserialization of HashJoinInfo would cause incompatibility between different versions.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57269833 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java — @@ -324,10 +328,13 @@ protected QueryPlan compileJoinQuery(StatementContext context, List<Object> bind QueryPlan plan = compileSingleFlatQuery(context, query, binds, asSubquery, !asSubquery && joinTable.isAllLeftJoin(), null, !table.isSubselect() && projectPKColumns ? tupleProjector : null, true); Expression postJoinFilterExpression = joinTable.compilePostFilterExpression(context, table); Integer limit = null; + Integer offset = null; if (!query.isAggregate() && !query.isDistinct() && query.getOrderBy().isEmpty()) { limit = plan.getLimit(); + offset = plan.getOffset(); } HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes, starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit); + HashJoinInfo joinInfo = new HashJoinInfo(projectedTable, joinIds, joinExpressions, joinTypes, + starJoinVector, tables, fieldPositions, postJoinFilterExpression, limit, offset); End diff – The "limit" in HashJoinInfo is a little different than the original SQL semantics. It specifies the max number of rows returned by a HashJoinRegionScanner so that it doesn't do too much unnecessary work, and its max rows per region, so in the end the client might get more rows than needed and will do a final limit operation (all done by ScanPlan, don't worry). So, here, I think the limit should be "offset + limit" and there is no need to add an extra "offset" field to HashJoinInfo. Btw, change in serialization/deserialization of HashJoinInfo would cause incompatibility between different versions.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user maryannxue commented on a diff in the pull request:

        https://github.com/apache/phoenix/pull/154#discussion_r57269271

        — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java —
        @@ -202,14 +204,30 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec
        }
        }
        }

        • + OffsetNode offset = select.getOffset();
          + if (offset != null) {
          + if (offsetRewrite == null)

          { + offsetRewrite = offset; + }

          else

          Unknown macro: { + Integer offsetValue = OffsetCompiler.compile(null, select); + Integer offsetValueSubselect = OffsetCompiler.compile(null, subselect); + if (offsetValue != null && offsetValueSubselect != null) { + offsetRewrite = offsetValue < offsetValueSubselect ? offset : offsetRewrite; + } else { + return select; + } + }

          + }
          +

            • End diff –

        Not sure if this would be correct. Suppose we have "select * from (select a from t offset 2 limit 8) offset 3", guess we should return the 5th row instead. If you consider the logic is too complicated to optimize at compiletime, you can simply quit flattening.

        Show
        githubbot ASF GitHub Bot added a comment - Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/154#discussion_r57269271 — Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/SubselectRewriter.java — @@ -202,14 +204,30 @@ private SelectStatement flatten(SelectStatement select, SelectStatement subselec } } } + OffsetNode offset = select.getOffset(); + if (offset != null) { + if (offsetRewrite == null) { + offsetRewrite = offset; + } else Unknown macro: { + Integer offsetValue = OffsetCompiler.compile(null, select); + Integer offsetValueSubselect = OffsetCompiler.compile(null, subselect); + if (offsetValue != null && offsetValueSubselect != null) { + offsetRewrite = offsetValue < offsetValueSubselect ? offset : offsetRewrite; + } else { + return select; + } + } + } + End diff – Not sure if this would be correct. Suppose we have "select * from (select a from t offset 2 limit 8) offset 3", guess we should return the 5th row instead. If you consider the logic is too complicated to optimize at compiletime, you can simply quit flattening.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user JamesRTaylor commented on the pull request:

        https://github.com/apache/phoenix/pull/154#issuecomment-200195853

        Would you mind reviewing this, @maryannxue as you'd be the best person to catch any corner cases missed, in particular around joins.

        Show
        githubbot ASF GitHub Bot added a comment - Github user JamesRTaylor commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-200195853 Would you mind reviewing this, @maryannxue as you'd be the best person to catch any corner cases missed, in particular around joins.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user ankitsinghal opened a pull request:

        https://github.com/apache/phoenix/pull/154

        PHOENIX-2722 support mysql limit,offset clauses

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/ankitsinghal/phoenix PHOENIX-2722

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/phoenix/pull/154.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #154


        commit 5d4e42dbd701da639c8d25789b03d3dc05244ba7
        Author: Ankit Singhal <ankitsinghal59@gmail.com>
        Date: 2016-03-22T08:11:22Z

        PHOENIX-2722 support mysql limit,offset clauses


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user ankitsinghal opened a pull request: https://github.com/apache/phoenix/pull/154 PHOENIX-2722 support mysql limit,offset clauses You can merge this pull request into a Git repository by running: $ git pull https://github.com/ankitsinghal/phoenix PHOENIX-2722 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/154.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #154 commit 5d4e42dbd701da639c8d25789b03d3dc05244ba7 Author: Ankit Singhal <ankitsinghal59@gmail.com> Date: 2016-03-22T08:11:22Z PHOENIX-2722 support mysql limit,offset clauses
        Hide
        ankit.singhal Ankit Singhal added a comment -

        I don't understand the need for this grammar change, as I thought this was about adding support for OFFSET?

        [ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
        

        In order to make OFFSET clause compatible with calcite, I need to add ROW|ROWS to it , and that will not be accurate until we support FETCH syntax for limit also as per SQL 2008.

        Regarding the white space changes.

        Sorry I din't know running command+shift+F on empty space results in formatting of whole file. I have reverted those changes in attached patch.

        Show
        ankit.singhal Ankit Singhal added a comment - I don't understand the need for this grammar change, as I thought this was about adding support for OFFSET? [ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ] In order to make OFFSET clause compatible with calcite, I need to add ROW|ROWS to it , and that will not be accurate until we support FETCH syntax for limit also as per SQL 2008. Regarding the white space changes. Sorry I din't know running command+shift+F on empty space results in formatting of whole file. I have reverted those changes in attached patch.
        Hide
        jamestaylor James Taylor added a comment - - edited

        I don't understand the need for this grammar change, as I thought this was about adding support for OFFSET?

        [ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ]
        

        Regarding the white space changes, we gave you a free pass before with the agreement that it'd get better next time. This patch is 3649 lines with a lot of them appearing to just be white space related or pure formatting. I think it's fine if a line is changing that the formatting can change (presuming it follows our conventions[1]), but I don't think the entire file should be reformatted. If you could make a pass through the patch and copy/paste the original code where you've made purely white space/formatting changes, that would be much appreciated.

        Here's one example:

        @@ -194,26 +183,25 @@ public class SubqueryRewriter extends ParseNodeRewriter {
                 if (isTopNode) {
                     topNode = null;
                 }
        -        
        +
                 ParseNode secondChild = l.get(1);
        -        if (!(secondChild instanceof SubqueryParseNode)) {
        -            return super.visitLeave(node, l);
        -        }
        -        
        -        SubqueryParseNode subqueryNode = (SubqueryParseNode) secondChild;
        +        if (!(secondChild instanceof SubqueryParseNode)) { return super.visitLeave(node, l); }
        +
        +        SubqueryParseNode subqueryNode = (SubqueryParseNode)secondChild;
                 SelectStatement subquery = fixSubqueryStatement(subqueryNode.getSelectNode());
                 String rhsTableAlias = ParseNodeFactory.createTempAlias();
        -        JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, rhsTableAlias);
        +        JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection,
        +                rhsTableAlias);
                 ParseNode where = subquery.getWhere() == null ? null : subquery.getWhere().accept(conditionExtractor);
                 if (where == subquery.getWhere()) { // non-correlated comparison subquery, add LIMIT 2, expectSingleRow = true
        -            subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(2)));
        +            subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(2)), null);
                     subqueryNode = NODE_FACTORY.subquery(subquery, true);
                     l = Lists.newArrayList(l.get(0), subqueryNode);
                     node = NODE_FACTORY.comparison(node.getFilterOp(), l.get(0), l.get(1));
                     return super.visitLeave(node, l);
                 }
        -        
        -        ParseNode rhsNode = null; 
        +
        +        ParseNode rhsNode = null;
                 boolean isGroupby = !subquery.getGroupBy().isEmpty();
                 boolean isAggregate = subquery.isAggregate();
                 List<AliasedNode> aliasedNodes = subquery.getSelect();
        @@ -226,52 +214,52 @@ public class SubqueryRewriter extends ParseNodeRewriter {
                     }
                     rhsNode = NODE_FACTORY.rowValueConstructor(nodes);
                 }
        -        
        +
                 List<AliasedNode> additionalSelectNodes = conditionExtractor.getAdditionalSelectNodes();
        -        List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1);        
        +        List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1);
                 selectNodes.add(NODE_FACTORY.aliasedNode(ParseNodeFactory.createTempAlias(), rhsNode));
                 selectNodes.addAll(additionalSelectNodes);
        -        
        +
                 if (!isAggregate) {
        -            subquery = NODE_FACTORY.select(subquery, subquery.isDistinct(), selectNodes, where);            
        +            subquery = NODE_FACTORY.select(subquery, subquery.isDistinct(), selectNodes, where);
                 } else {
        -            List<ParseNode> groupbyNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + subquery.getGroupBy().size());
        +            List<ParseNode> groupbyNodes = Lists
        +                    .newArrayListWithExpectedSize(additionalSelectNodes.size() + subquery.getGroupBy().size());
                     for (AliasedNode aliasedNode : additionalSelectNodes) {
                         groupbyNodes.add(aliasedNode.getNode());
                     }
                     groupbyNodes.addAll(subquery.getGroupBy());
                     subquery = NODE_FACTORY.select(subquery, subquery.isDistinct(), selectNodes, where, groupbyNodes, true);
                 }
        -        
        +
                 ParseNode onNode = conditionExtractor.getJoinCondition();
                 TableNode rhsTable = NODE_FACTORY.derivedTable(rhsTableAlias, subquery);
                 JoinType joinType = isTopNode ? JoinType.Inner : JoinType.Left;
        -        ParseNode ret = NODE_FACTORY.comparison(node.getFilterOp(), l.get(0), NODE_FACTORY.column(NODE_FACTORY.table(null, rhsTableAlias), selectNodes.get(0).getAlias(), null));
        +        ParseNode ret = NODE_FACTORY.comparison(node.getFilterOp(), l.get(0),
        +                NODE_FACTORY.column(NODE_FACTORY.table(null, rhsTableAlias), selectNodes.get(0).getAlias(), null));
                 tableNode = NODE_FACTORY.join(joinType, tableNode, rhsTable, onNode, !isAggregate || isGroupby);
        -        
        +
                 return ret;
             }
         
             @Override
             public ParseNode visitLeave(ArrayAnyComparisonNode node, List<ParseNode> l) throws SQLException {
                 List<ParseNode> children = leaveArrayComparisonNode(node, l);
        -        if (children == l)
        -            return super.visitLeave(node, l);
        -        
        -        node = NODE_FACTORY.arrayAny(children.get(0), (ComparisonParseNode) children.get(1));
        +        if (children == l) return super.visitLeave(node, l);
        +
        +        node = NODE_FACTORY.arrayAny(children.get(0), (ComparisonParseNode)children.get(1));
                 return node;
             }
         
             @Override
             public ParseNode visitLeave(ArrayAllComparisonNode node, List<ParseNode> l) throws SQLException {
                 List<ParseNode> children = leaveArrayComparisonNode(node, l);
        -        if (children == l)
        -            return super.visitLeave(node, l);
        -        
        -        node = NODE_FACTORY.arrayAll(children.get(0), (ComparisonParseNode) children.get(1));
        +        if (children == l) return super.visitLeave(node, l);
        +
        +        node = NODE_FACTORY.arrayAll(children.get(0), (ComparisonParseNode)children.get(1));
                 return node;
             }
        -    
        +
             protected List<ParseNode> leaveArrayComparisonNode(ParseNode node, List<ParseNode> l) throws SQLException {
                 boolean isTopNode = topNode == node;
                 if (isTopNode) {
        @@ -279,20 +267,19 @@ public class SubqueryRewriter extends ParseNodeRewriter {
                 }
         
                 ParseNode firstChild = l.get(0);
        -        if (!(firstChild instanceof SubqueryParseNode)) {
        -            return l;
        -        }
        -        
        -        SubqueryParseNode subqueryNode = (SubqueryParseNode) firstChild;
        +        if (!(firstChild instanceof SubqueryParseNode)) { return l; }
        +
        +        SubqueryParseNode subqueryNode = (SubqueryParseNode)firstChild;
                 SelectStatement subquery = fixSubqueryStatement(subqueryNode.getSelectNode());
                 String rhsTableAlias = ParseNodeFactory.createTempAlias();
        -        JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, rhsTableAlias);
        +        JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection,
        +                rhsTableAlias);
                 ParseNode where = subquery.getWhere() == null ? null : subquery.getWhere().accept(conditionExtractor);
                 if (where == subquery.getWhere()) { // non-correlated any/all comparison subquery
                     return l;
                 }
        -        
        -        ParseNode rhsNode = null; 
        +
        +        ParseNode rhsNode = null;
                 boolean isNonGroupByAggregate = subquery.getGroupBy().isEmpty() && subquery.isAggregate();
                 List<AliasedNode> aliasedNodes = subquery.getSelect();
                 String derivedTableAlias = null;
        @@ -300,50 +287,61 @@ public class SubqueryRewriter extends ParseNodeRewriter {
                     derivedTableAlias = ParseNodeFactory.createTempAlias();
                     aliasedNodes = fixAliasedNodes(aliasedNodes, false);
                 }
        -        
        +
                 if (aliasedNodes.size() == 1) {
        -            rhsNode = derivedTableAlias == null ? aliasedNodes.get(0).getNode() : NODE_FACTORY.column(NODE_FACTORY.table(null, derivedTableAlias), aliasedNodes.get(0).getAlias(), null);
        +            rhsNode = derivedTableAlias == null ? aliasedNodes.get(0).getNode()
        +                    : NODE_FACTORY.column(NODE_FACTORY.table(null, derivedTableAlias), aliasedNodes.get(0).getAlias(),
        +                            null);
                 } else {
                     List<ParseNode> nodes = Lists.<ParseNode> newArrayListWithExpectedSize(aliasedNodes.size());
                     for (AliasedNode aliasedNode : aliasedNodes) {
        -                nodes.add(derivedTableAlias == null ? aliasedNode.getNode() : NODE_FACTORY.column(NODE_FACTORY.table(null, derivedTableAlias), aliasedNode.getAlias(), null));
        +                nodes.add(derivedTableAlias == null ? aliasedNode.getNode()
        +                        : NODE_FACTORY.column(NODE_FACTORY.table(null, derivedTableAlias), aliasedNode.getAlias(),
        +                                null));
                     }
                     rhsNode = NODE_FACTORY.rowValueConstructor(nodes);
                 }
        -        
        +
                 if (!isNonGroupByAggregate) {
                     rhsNode = NODE_FACTORY.function(DistinctValueAggregateFunction.NAME, Collections.singletonList(rhsNode));
                 }
        -        
        +
                 List<AliasedNode> additionalSelectNodes = conditionExtractor.getAdditionalSelectNodes();
        -        List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1);        
        +        List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1);
                 selectNodes.add(NODE_FACTORY.aliasedNode(ParseNodeFactory.createTempAlias(), rhsNode));
                 selectNodes.addAll(additionalSelectNodes);
                 List<ParseNode> groupbyNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size());
                 for (AliasedNode aliasedNode : additionalSelectNodes) {
                     groupbyNodes.add(aliasedNode.getNode());
                 }
        -        
        +
                 if (derivedTableAlias == null) {
                     subquery = NODE_FACTORY.select(subquery, false, selectNodes, where, groupbyNodes, true);
                 } else {
        -            List<ParseNode> derivedTableGroupBy = Lists.newArrayListWithExpectedSize(subquery.getGroupBy().size() + groupbyNodes.size());
        +            List<ParseNode> derivedTableGroupBy = Lists
        +                    .newArrayListWithExpectedSize(subquery.getGroupBy().size() + groupbyNodes.size());
                     derivedTableGroupBy.addAll(groupbyNodes);
                     derivedTableGroupBy.addAll(subquery.getGroupBy());
        -            List<AliasedNode> derivedTableSelect = Lists.newArrayListWithExpectedSize(aliasedNodes.size() + selectNodes.size() - 1);
        +            List<AliasedNode> derivedTableSelect = Lists
        +                    .newArrayListWithExpectedSize(aliasedNodes.size() + selectNodes.size() - 1);
                     derivedTableSelect.addAll(aliasedNodes);
                     for (int i = 1; i < selectNodes.size(); i++) {
                         AliasedNode aliasedNode = selectNodes.get(i);
                         String alias = ParseNodeFactory.createTempAlias();
                         derivedTableSelect.add(NODE_FACTORY.aliasedNode(alias, aliasedNode.getNode()));
        -                aliasedNode = NODE_FACTORY.aliasedNode(aliasedNode.getAlias(), NODE_FACTORY.column(NODE_FACTORY.table(null, derivedTableAlias), alias, null));
        +                aliasedNode = NODE_FACTORY.aliasedNode(aliasedNode.getAlias(),
        +                        NODE_FACTORY.column(NODE_FACTORY.table(null, derivedTableAlias), alias, null));
                         selectNodes.set(i, aliasedNode);
                         groupbyNodes.set(i - 1, aliasedNode.getNode());
                     }
        -            SelectStatement derivedTableStmt = NODE_FACTORY.select(subquery, subquery.isDistinct(), derivedTableSelect, where, derivedTableGroupBy, true);
        -            subquery = NODE_FACTORY.select(NODE_FACTORY.derivedTable(derivedTableAlias, derivedTableStmt), subquery.getHint(), false, selectNodes, null, groupbyNodes, null, Collections.<OrderByNode> emptyList(), null, subquery.getBindCount(), true, false, Collections.<SelectStatement>emptyList(), subquery.getUdfParseNodes());
        +            SelectStatement derivedTableStmt = NODE_FACTORY.select(subquery, subquery.isDistinct(), derivedTableSelect,
        +                    where, derivedTableGroupBy, true);
        +            subquery = NODE_FACTORY.select(NODE_FACTORY.derivedTable(derivedTableAlias, derivedTableStmt),
        +                    subquery.getHint(), false, selectNodes, null, groupbyNodes, null,
        +                    Collections.<OrderByNode> emptyList(), null, null, subquery.getBindCount(), true, false,
        +                    Collections.<SelectStatement> emptyList(), subquery.getUdfParseNodes());
                 }
        -        
        +
                 ParseNode onNode = conditionExtractor.getJoinCondition();
                 TableNode rhsTable = NODE_FACTORY.derivedTable(rhsTableAlias, subquery);
                 JoinType joinType = isTopNode ? JoinType.Inner : JoinType.Left;
        @@ -351,46 +349,51 @@ public class SubqueryRewriter extends ParseNodeRewriter {
         
                 firstChild = NODE_FACTORY.column(NODE_FACTORY.table(null, rhsTableAlias), selectNodes.get(0).getAlias(), null);
                 if (isNonGroupByAggregate) {
        -            firstChild = NODE_FACTORY.upsertStmtArrayNode(Collections.singletonList(firstChild)); 
        +            firstChild = NODE_FACTORY.upsertStmtArrayNode(Collections.singletonList(firstChild));
                 }
        -        ComparisonParseNode secondChild = (ComparisonParseNode) l.get(1);
        -        secondChild = NODE_FACTORY.comparison(secondChild.getFilterOp(), secondChild.getLHS(), NODE_FACTORY.elementRef(Lists.newArrayList(firstChild, NODE_FACTORY.literal(1))));
        -        
        +        ComparisonParseNode secondChild = (ComparisonParseNode)l.get(1);
        +        secondChild = NODE_FACTORY.comparison(secondChild.getFilterOp(), secondChild.getLHS(),
        +                NODE_FACTORY.elementRef(Lists.newArrayList(firstChild, NODE_FACTORY.literal(1))));
        +
                 return Lists.newArrayList(firstChild, secondChild);
             }
        -    
        +
             private SelectStatement fixSubqueryStatement(SelectStatement select) {
        -        if (!select.isUnion())
        -            return select;
        -        
        +        if (!select.isUnion()) return select;
        +
                 // Wrap as a derived table.
        -        return NODE_FACTORY.select(NODE_FACTORY.derivedTable(ParseNodeFactory.createTempAlias(), select), HintNode.EMPTY_HINT_NODE, false, select.getSelect(), null, null, null, null, null, select.getBindCount(), false, false, Collections.<SelectStatement> emptyList(), select.getUdfParseNodes());
        +        return NODE_FACTORY.select(NODE_FACTORY.derivedTable(ParseNodeFactory.createTempAlias(), select),
        +                HintNode.EMPTY_HINT_NODE, false, select.getSelect(), null, null, null, null, null, null,
        +                select.getBindCount(), false, false, Collections.<SelectStatement> emptyList(),
        +                select.getUdfParseNodes());
             }
        -    
        +
        
        Show
        jamestaylor James Taylor added a comment - - edited I don't understand the need for this grammar change, as I thought this was about adding support for OFFSET? [ FETCH { FIRST | NEXT } [ count ] { ROW | ROWS } ONLY ] Regarding the white space changes, we gave you a free pass before with the agreement that it'd get better next time. This patch is 3649 lines with a lot of them appearing to just be white space related or pure formatting. I think it's fine if a line is changing that the formatting can change (presuming it follows our conventions [1] ), but I don't think the entire file should be reformatted. If you could make a pass through the patch and copy/paste the original code where you've made purely white space/formatting changes, that would be much appreciated. Here's one example: @@ -194,26 +183,25 @@ public class SubqueryRewriter extends ParseNodeRewriter { if (isTopNode) { topNode = null ; } - + ParseNode secondChild = l.get(1); - if (!(secondChild instanceof SubqueryParseNode)) { - return super .visitLeave(node, l); - } - - SubqueryParseNode subqueryNode = (SubqueryParseNode) secondChild; + if (!(secondChild instanceof SubqueryParseNode)) { return super .visitLeave(node, l); } + + SubqueryParseNode subqueryNode = (SubqueryParseNode)secondChild; SelectStatement subquery = fixSubqueryStatement(subqueryNode.getSelectNode()); String rhsTableAlias = ParseNodeFactory.createTempAlias(); - JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, rhsTableAlias); + JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, + rhsTableAlias); ParseNode where = subquery.getWhere() == null ? null : subquery.getWhere().accept(conditionExtractor); if (where == subquery.getWhere()) { // non-correlated comparison subquery, add LIMIT 2, expectSingleRow = true - subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(2))); + subquery = NODE_FACTORY.select(subquery, NODE_FACTORY.limit(NODE_FACTORY.literal(2)), null ); subqueryNode = NODE_FACTORY.subquery(subquery, true ); l = Lists.newArrayList(l.get(0), subqueryNode); node = NODE_FACTORY.comparison(node.getFilterOp(), l.get(0), l.get(1)); return super .visitLeave(node, l); } - - ParseNode rhsNode = null ; + + ParseNode rhsNode = null ; boolean isGroupby = !subquery.getGroupBy().isEmpty(); boolean isAggregate = subquery.isAggregate(); List<AliasedNode> aliasedNodes = subquery.getSelect(); @@ -226,52 +214,52 @@ public class SubqueryRewriter extends ParseNodeRewriter { } rhsNode = NODE_FACTORY.rowValueConstructor(nodes); } - + List<AliasedNode> additionalSelectNodes = conditionExtractor.getAdditionalSelectNodes(); - List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1); + List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1); selectNodes.add(NODE_FACTORY.aliasedNode(ParseNodeFactory.createTempAlias(), rhsNode)); selectNodes.addAll(additionalSelectNodes); - + if (!isAggregate) { - subquery = NODE_FACTORY.select(subquery, subquery.isDistinct(), selectNodes, where); + subquery = NODE_FACTORY.select(subquery, subquery.isDistinct(), selectNodes, where); } else { - List<ParseNode> groupbyNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + subquery.getGroupBy().size()); + List<ParseNode> groupbyNodes = Lists + .newArrayListWithExpectedSize(additionalSelectNodes.size() + subquery.getGroupBy().size()); for (AliasedNode aliasedNode : additionalSelectNodes) { groupbyNodes.add(aliasedNode.getNode()); } groupbyNodes.addAll(subquery.getGroupBy()); subquery = NODE_FACTORY.select(subquery, subquery.isDistinct(), selectNodes, where, groupbyNodes, true ); } - + ParseNode onNode = conditionExtractor.getJoinCondition(); TableNode rhsTable = NODE_FACTORY.derivedTable(rhsTableAlias, subquery); JoinType joinType = isTopNode ? JoinType.Inner : JoinType.Left; - ParseNode ret = NODE_FACTORY.comparison(node.getFilterOp(), l.get(0), NODE_FACTORY.column(NODE_FACTORY.table( null , rhsTableAlias), selectNodes.get(0).getAlias(), null )); + ParseNode ret = NODE_FACTORY.comparison(node.getFilterOp(), l.get(0), + NODE_FACTORY.column(NODE_FACTORY.table( null , rhsTableAlias), selectNodes.get(0).getAlias(), null )); tableNode = NODE_FACTORY.join(joinType, tableNode, rhsTable, onNode, !isAggregate || isGroupby); - + return ret; } @Override public ParseNode visitLeave(ArrayAnyComparisonNode node, List<ParseNode> l) throws SQLException { List<ParseNode> children = leaveArrayComparisonNode(node, l); - if (children == l) - return super .visitLeave(node, l); - - node = NODE_FACTORY.arrayAny(children.get(0), (ComparisonParseNode) children.get(1)); + if (children == l) return super .visitLeave(node, l); + + node = NODE_FACTORY.arrayAny(children.get(0), (ComparisonParseNode)children.get(1)); return node; } @Override public ParseNode visitLeave(ArrayAllComparisonNode node, List<ParseNode> l) throws SQLException { List<ParseNode> children = leaveArrayComparisonNode(node, l); - if (children == l) - return super .visitLeave(node, l); - - node = NODE_FACTORY.arrayAll(children.get(0), (ComparisonParseNode) children.get(1)); + if (children == l) return super .visitLeave(node, l); + + node = NODE_FACTORY.arrayAll(children.get(0), (ComparisonParseNode)children.get(1)); return node; } - + protected List<ParseNode> leaveArrayComparisonNode(ParseNode node, List<ParseNode> l) throws SQLException { boolean isTopNode = topNode == node; if (isTopNode) { @@ -279,20 +267,19 @@ public class SubqueryRewriter extends ParseNodeRewriter { } ParseNode firstChild = l.get(0); - if (!(firstChild instanceof SubqueryParseNode)) { - return l; - } - - SubqueryParseNode subqueryNode = (SubqueryParseNode) firstChild; + if (!(firstChild instanceof SubqueryParseNode)) { return l; } + + SubqueryParseNode subqueryNode = (SubqueryParseNode)firstChild; SelectStatement subquery = fixSubqueryStatement(subqueryNode.getSelectNode()); String rhsTableAlias = ParseNodeFactory.createTempAlias(); - JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, rhsTableAlias); + JoinConditionExtractor conditionExtractor = new JoinConditionExtractor(subquery, resolver, connection, + rhsTableAlias); ParseNode where = subquery.getWhere() == null ? null : subquery.getWhere().accept(conditionExtractor); if (where == subquery.getWhere()) { // non-correlated any/all comparison subquery return l; } - - ParseNode rhsNode = null ; + + ParseNode rhsNode = null ; boolean isNonGroupByAggregate = subquery.getGroupBy().isEmpty() && subquery.isAggregate(); List<AliasedNode> aliasedNodes = subquery.getSelect(); String derivedTableAlias = null ; @@ -300,50 +287,61 @@ public class SubqueryRewriter extends ParseNodeRewriter { derivedTableAlias = ParseNodeFactory.createTempAlias(); aliasedNodes = fixAliasedNodes(aliasedNodes, false ); } - + if (aliasedNodes.size() == 1) { - rhsNode = derivedTableAlias == null ? aliasedNodes.get(0).getNode() : NODE_FACTORY.column(NODE_FACTORY.table( null , derivedTableAlias), aliasedNodes.get(0).getAlias(), null ); + rhsNode = derivedTableAlias == null ? aliasedNodes.get(0).getNode() + : NODE_FACTORY.column(NODE_FACTORY.table( null , derivedTableAlias), aliasedNodes.get(0).getAlias(), + null ); } else { List<ParseNode> nodes = Lists.<ParseNode> newArrayListWithExpectedSize(aliasedNodes.size()); for (AliasedNode aliasedNode : aliasedNodes) { - nodes.add(derivedTableAlias == null ? aliasedNode.getNode() : NODE_FACTORY.column(NODE_FACTORY.table( null , derivedTableAlias), aliasedNode.getAlias(), null )); + nodes.add(derivedTableAlias == null ? aliasedNode.getNode() + : NODE_FACTORY.column(NODE_FACTORY.table( null , derivedTableAlias), aliasedNode.getAlias(), + null )); } rhsNode = NODE_FACTORY.rowValueConstructor(nodes); } - + if (!isNonGroupByAggregate) { rhsNode = NODE_FACTORY.function(DistinctValueAggregateFunction.NAME, Collections.singletonList(rhsNode)); } - + List<AliasedNode> additionalSelectNodes = conditionExtractor.getAdditionalSelectNodes(); - List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1); + List<AliasedNode> selectNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size() + 1); selectNodes.add(NODE_FACTORY.aliasedNode(ParseNodeFactory.createTempAlias(), rhsNode)); selectNodes.addAll(additionalSelectNodes); List<ParseNode> groupbyNodes = Lists.newArrayListWithExpectedSize(additionalSelectNodes.size()); for (AliasedNode aliasedNode : additionalSelectNodes) { groupbyNodes.add(aliasedNode.getNode()); } - + if (derivedTableAlias == null ) { subquery = NODE_FACTORY.select(subquery, false , selectNodes, where, groupbyNodes, true ); } else { - List<ParseNode> derivedTableGroupBy = Lists.newArrayListWithExpectedSize(subquery.getGroupBy().size() + groupbyNodes.size()); + List<ParseNode> derivedTableGroupBy = Lists + .newArrayListWithExpectedSize(subquery.getGroupBy().size() + groupbyNodes.size()); derivedTableGroupBy.addAll(groupbyNodes); derivedTableGroupBy.addAll(subquery.getGroupBy()); - List<AliasedNode> derivedTableSelect = Lists.newArrayListWithExpectedSize(aliasedNodes.size() + selectNodes.size() - 1); + List<AliasedNode> derivedTableSelect = Lists + .newArrayListWithExpectedSize(aliasedNodes.size() + selectNodes.size() - 1); derivedTableSelect.addAll(aliasedNodes); for ( int i = 1; i < selectNodes.size(); i++) { AliasedNode aliasedNode = selectNodes.get(i); String alias = ParseNodeFactory.createTempAlias(); derivedTableSelect.add(NODE_FACTORY.aliasedNode(alias, aliasedNode.getNode())); - aliasedNode = NODE_FACTORY.aliasedNode(aliasedNode.getAlias(), NODE_FACTORY.column(NODE_FACTORY.table( null , derivedTableAlias), alias, null )); + aliasedNode = NODE_FACTORY.aliasedNode(aliasedNode.getAlias(), + NODE_FACTORY.column(NODE_FACTORY.table( null , derivedTableAlias), alias, null )); selectNodes.set(i, aliasedNode); groupbyNodes.set(i - 1, aliasedNode.getNode()); } - SelectStatement derivedTableStmt = NODE_FACTORY.select(subquery, subquery.isDistinct(), derivedTableSelect, where, derivedTableGroupBy, true ); - subquery = NODE_FACTORY.select(NODE_FACTORY.derivedTable(derivedTableAlias, derivedTableStmt), subquery.getHint(), false , selectNodes, null , groupbyNodes, null , Collections.<OrderByNode> emptyList(), null , subquery.getBindCount(), true , false , Collections.<SelectStatement>emptyList(), subquery.getUdfParseNodes()); + SelectStatement derivedTableStmt = NODE_FACTORY.select(subquery, subquery.isDistinct(), derivedTableSelect, + where, derivedTableGroupBy, true ); + subquery = NODE_FACTORY.select(NODE_FACTORY.derivedTable(derivedTableAlias, derivedTableStmt), + subquery.getHint(), false , selectNodes, null , groupbyNodes, null , + Collections.<OrderByNode> emptyList(), null , null , subquery.getBindCount(), true , false , + Collections.<SelectStatement> emptyList(), subquery.getUdfParseNodes()); } - + ParseNode onNode = conditionExtractor.getJoinCondition(); TableNode rhsTable = NODE_FACTORY.derivedTable(rhsTableAlias, subquery); JoinType joinType = isTopNode ? JoinType.Inner : JoinType.Left; @@ -351,46 +349,51 @@ public class SubqueryRewriter extends ParseNodeRewriter { firstChild = NODE_FACTORY.column(NODE_FACTORY.table( null , rhsTableAlias), selectNodes.get(0).getAlias(), null ); if (isNonGroupByAggregate) { - firstChild = NODE_FACTORY.upsertStmtArrayNode(Collections.singletonList(firstChild)); + firstChild = NODE_FACTORY.upsertStmtArrayNode(Collections.singletonList(firstChild)); } - ComparisonParseNode secondChild = (ComparisonParseNode) l.get(1); - secondChild = NODE_FACTORY.comparison(secondChild.getFilterOp(), secondChild.getLHS(), NODE_FACTORY.elementRef(Lists.newArrayList(firstChild, NODE_FACTORY.literal(1)))); - + ComparisonParseNode secondChild = (ComparisonParseNode)l.get(1); + secondChild = NODE_FACTORY.comparison(secondChild.getFilterOp(), secondChild.getLHS(), + NODE_FACTORY.elementRef(Lists.newArrayList(firstChild, NODE_FACTORY.literal(1)))); + return Lists.newArrayList(firstChild, secondChild); } - + private SelectStatement fixSubqueryStatement(SelectStatement select) { - if (!select.isUnion()) - return select; - + if (!select.isUnion()) return select; + // Wrap as a derived table. - return NODE_FACTORY.select(NODE_FACTORY.derivedTable(ParseNodeFactory.createTempAlias(), select), HintNode.EMPTY_HINT_NODE, false , select.getSelect(), null , null , null , null , null , select.getBindCount(), false , false , Collections.<SelectStatement> emptyList(), select.getUdfParseNodes()); + return NODE_FACTORY.select(NODE_FACTORY.derivedTable(ParseNodeFactory.createTempAlias(), select), + HintNode.EMPTY_HINT_NODE, false , select.getSelect(), null , null , null , null , null , null , + select.getBindCount(), false , false , Collections.<SelectStatement> emptyList(), + select.getUdfParseNodes()); } - +
        Hide
        ankit.singhal Ankit Singhal added a comment - - edited

        I think some are appearing to be only whitespace related changes but they all have one more parameter passed(Integer/OffsetNode offset).
        should I not change the formatting of existing line if we just make a small addition to it?

        Show
        ankit.singhal Ankit Singhal added a comment - - edited I think some are appearing to be only whitespace related changes but they all have one more parameter passed(Integer/OffsetNode offset). should I not change the formatting of existing line if we just make a small addition to it?
        Hide
        jamestaylor James Taylor added a comment -

        Ankit Singhal - thanks for the patch. There are too many whitespace only changes. Please revert all formatting changes to make this easier to review.

        Show
        jamestaylor James Taylor added a comment - Ankit Singhal - thanks for the patch. There are too many whitespace only changes. Please revert all formatting changes to make this easier to review.
        Hide
        ankit.singhal Ankit Singhal added a comment -

        thanks James Taylor for the comments.
        can you please review the attached patch.

        Attached patch includes both of your input :-

        • Pushing offset to server for serial queries(non-order by and non-aggregate)
        • SQL syntax compatible with calcite.
        Show
        ankit.singhal Ankit Singhal added a comment - thanks James Taylor for the comments. can you please review the attached patch. Attached patch includes both of your input :- Pushing offset to server for serial queries(non-order by and non-aggregate) SQL syntax compatible with calcite.
        Hide
        jamestaylor James Taylor added a comment -

        Also, if you plan to pursue this, please make sure the syntax as compatible with Calcite.

        Show
        jamestaylor James Taylor added a comment - Also, if you plan to pursue this, please make sure the syntax as compatible with Calcite.
        Hide
        jamestaylor James Taylor added a comment -

        You could force a query with an offset to run serially and at a minimum push the offset to the server so that the coprocessor can do the next() rather than return all the data back to the client (at least for the non order by case).

        Show
        jamestaylor James Taylor added a comment - You could force a query with an offset to run serially and at a minimum push the offset to the server so that the coprocessor can do the next() rather than return all the data back to the client (at least for the non order by case).
        Hide
        ankit.singhal Ankit Singhal added a comment -

        Although offset for queries threaded in parallel(including orderby) would not be optimized as serial queries because offset can not pushed down to servers for them.
        but it's just help the user not to call next() offset number of times instead get some optimization abstracted under a SQL construct.

        Users can use it for random access of pages or paged queries wherever RVC is not possible.

        what about including this construct with the details about the implementation in the documentation and user can use it as per the optimization?

        Show
        ankit.singhal Ankit Singhal added a comment - Although offset for queries threaded in parallel(including orderby) would not be optimized as serial queries because offset can not pushed down to servers for them. but it's just help the user not to call next() offset number of times instead get some optimization abstracted under a SQL construct. Users can use it for random access of pages or paged queries wherever RVC is not possible. what about including this construct with the details about the implementation in the documentation and user can use it as per the optimization?
        Hide
        jamestaylor James Taylor added a comment -

        We already support LIMIT. It's possible we could support OFFSET for serial queries, but it's not clear how useful this is - seems like we have other more important things to do (like finishing the Calcite integration).

        Show
        jamestaylor James Taylor added a comment - We already support LIMIT. It's possible we could support OFFSET for serial queries, but it's not clear how useful this is - seems like we have other more important things to do (like finishing the Calcite integration).

          People

          • Assignee:
            ankit.singhal Ankit Singhal
            Reporter:
            ankit.singhal Ankit Singhal
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development