PHOENIX-933 Local index support to Phoenix #1

chrajeshbabu · 2014-07-04T10:53:05Z

No description provided.

…ndex table regions (Rajeshbabu)

… table (Rajeshbabu)

…amesTaylor)

…vering local index available (Rajeshbabu)

…able (Rajeshbabu)

…ot already there

…when query condition involves leading columns in local index (Rajeshbabu)

JamesRTaylor · 2014-07-09T14:41:13Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/index/MutableIndexIT.java

-                    + " INCLUDE (long_col1, long_col2)";
+            String ddl = null;
+            if(localIndex){
+                ddl = "CREATE INDEX " + INDEX_TABLE_NAME + " ON " + DATA_TABLE_FULL_NAME


Should this be CREATE LOCAL INDEX?

Somehow missed this. Corrected.

Looks like the if and the else branch match. If that's expected/correct, can you get rid of the if statement?

This is the actual code. Earlier by mistake I missed to add LOCAL in if branch so if else branches are same.
if (localIndex) {
ddl = "CREATE LOCAL INDEX " + INDEX_TABLE_NAME + " ON " + DATA_TABLE_FULL_NAME + " (date_col)";
} else {
ddl = "CREATE INDEX " + INDEX_TABLE_NAME + " ON " + DATA_TABLE_FULL_NAME + " (date_col)";
}

Now I have corrected it.

JamesRTaylor · 2014-07-09T15:25:05Z

Thanks for the pull, @chrajeshbabu. Nice work! Looks like you need to rebase as there are merge conflicts currently.

JamesRTaylor · 2014-07-09T15:43:07Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java

@@ -100,7 +109,7 @@ public static void serializeIntoScan(Scan scan, int thresholdBytes, int limit, L
        }
    }

-    public static OrderedResultIterator deserializeFromScan(Scan scan, RegionScanner s) {
+    public static OrderedResultIterator deserializeFromScan(Scan scan, RegionScanner s, int offset) {


There's a bit more you need to do to handle ORDER BY correctly. It'd be for the case in which a data column was referenced in the ORDER BY while the index table is being used to satisfy the query. Not here, but in OrderedResultIterator. In getResultIterator, in the beginning of the for loop, you'll need to wrap the result in the same way if there are dataColumns (i.e. call IndexUtil.wrapResultUsingOffset)

for (Tuple result = delegate.next(); result != null; result = delegate.next()) { int pos = 0; ImmutableBytesWritable[] sortKeys = new ImmutableBytesWritable[numSortKeys]; for (Expression expression : expressions) { final ImmutableBytesWritable sortKey = new ImmutableBytesWritable(); boolean evaluated = expression.evaluate(result, sortKey); // set the sort key that failed to get evaluated with null sortKeys[pos++] = evaluated && sortKey.getLength() > 0 ? sortKey : null; } queueEntries.add(new ResultEntry(sortKeys, result)); }

Without this, the table data column expressions that aren't in the local index will fail to evaluate. You should add a test for this too.

I think the cleanest way to handle this is by wrapping the ResultIterator passed into the OrderedResultIterator. Just create a new ResultIterator that delegates to the original one and does the wrapping necessary. Then you won't need to change OrderedResultIterator at all.

Actually, nix this, as the innerScanner you pass to OrderedResultIterator already does all of this. You likely already have a test for the ORDER BY case I mentioned, but if not, please add one.

This I will verify James.

bq. There's a bit more you need to do to handle ORDER BY correctly. It'd be for the case in which a data column was referenced in the ORDER BY while the index table is being used to satisfy the query.
This is working fine James. I have added test case.

Excellent. Would you mind updating this pull request? I think it's ready to get committed, no?

JamesRTaylor · 2014-07-09T16:12:18Z

Excellent job, @chrajeshbabu! A few minor issues, but overall this is great! Looking forward to seeing how it performs!

chrajeshbabu · 2014-07-12T01:34:54Z

Thanks for review @JamesRTaylor. I have resolved the conflicts and handled all the comments locally. I will submit it once I verify OrderedResultIterator scenario.

JamesRTaylor · 2014-07-12T08:03:58Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/BaseTenantSpecificViewIndexIT.java

        assertFalse(rs.next());
    }
+


This code looks familiar. If it matches original (I think from QueryIT), can you move it into the base test class instead of copy/paste?

JamesRTaylor · 2014-07-12T19:55:26Z

...x-core/src/main/java/org/apache/phoenix/iterate/SkipRangeParallelIteratorRegionSplitter.java

@@ -54,7 +55,8 @@ protected SkipRangeParallelIteratorRegionSplitter(StatementContext context, Tabl

    public List<HRegionLocation> filterRegions(List<HRegionLocation> allTableRegions, final ScanRanges ranges) {
        Iterable<HRegionLocation> regions;
-        if (ranges == ScanRanges.EVERYTHING) {
+        if (ranges == ScanRanges.EVERYTHING


This shouldn't be necessary and will cause the skip scan not to be used (which will be horrible for point queries). It should work fine to do the skip scan for each region (much better than doing a full region scan for every region).

Can you remove it and add a test that does a point lookup with multiple values (for example, a IN clause with multiple values for an indexed column)?

Even with the change skip scan will be used James. The change is required because the key ranges generated by compiler won't be in the local index regions key range because local index rows have prefixed region start key extra. Without the change mostly no region will be selected for scanning.

QueryIT#testSimpleInListStatement is the test case verifies the same.
Here is the explain query result.

CLIENT PARALLEL 4-WAY SKIP SCAN ON 2 KEYS OVER _LOCAL_IDX_ATABLE [-32768,2] - [-32768,4]
SERVER FILTER BY FIRST KEY ONLY AND ORGANIZATION_ID = '00D300000000XHP'
CLIENT MERGE SORT

JamesRTaylor · 2014-07-12T22:36:45Z

Related to the revert of the ScanRanges change, you'll need to make this change to prevent the SkipRangeParallelIteratorRegionSplitter from being used (as you always need to scan all regions for a local index). Cleanest might be to just implement a simple ParallelIteratorRegionSplitter for use when a local index is used that just returns all regions:

public class ParallelIteratorRegionSplitterFactory {

    public static ParallelIteratorRegionSplitter getSplitter(StatementContext context, TableRef table, HintNode hintNode) throws SQLException {
        if (!isLocalIndex && context.getScanRanges().useSkipScanFilter()) {
            return SkipRangeParallelIteratorRegionSplitter.getInstance(context, table, hintNode);
        }
        return DefaultParallelIteratorRegionSplitter.getInstance(context, table, hintNode);
    }
}

chrajeshbabu · 2014-07-13T12:29:09Z

bq. Cleanest might be to just implement a simple ParallelIteratorRegionSplitter for use when a local index is used that just returns all regions:
I will add new ParallelIteratorRegionSplitter for local index and remove the unnecessary changes in SkipRangeParallelIteratorRegionSplitter/DefaultParallelIteratorRegionSplitter.

Then I will submit another pull request.

Thanks @JamesRTaylor

JamesRTaylor · 2014-07-13T12:54:25Z

Sounds good, @chrajeshbabu. Thanks!

#1632) * PHOENIX-6897 Filters on unverified index rows return wrong result (#1597) * PHOENIX-6897 Filters on unverified index rows return wrong result * Fixed checkstyle and missing license warnings * Addressed review comments

JamesRTaylor and others added 10 commits April 16, 2014 18:28

Add local index to grammar and metadata

73e1c08

PHOENIX-936 Custom load balancer to colocate user table regions and i…

6110452

…ndex table regions (Rajeshbabu)

PHOENIX-935 create local index table with the same split keys of user…

492f775

… table (Rajeshbabu)

PHOENIX-935 create local index table with the same split keys of user…

ef50aab

… table (Rajeshbabu)

PHOENIX-955 Skip region start key at beginning of local index rows (J…

360f5a1

…amesTaylor)

PHOENIX-937 Handle puts on local index table (Rajeshbabu)

276de4a

PHOENIX-994 Handle scans on local index table in case any best fit co…

16ad190

…vering local index available (Rajeshbabu)

PHOENIX-1004 'drop index' should delete index data from local index t…

a567801

…able (Rajeshbabu)

PHOENIX-1038 Dynamically add INDEX_TYPE column to SYSTEM.CATALOG if n…

aa2559f

…ot already there

PHOENIX-1015 Support joining back to data table row from local index …

6e1fcc8

…when query condition involves leading columns in local index (Rajeshbabu)

chrajeshbabu changed the title ~~Local index~~ PHOENIX-933 Local index support to Phoenix Jul 7, 2014

JamesRTaylor reviewed Jul 9, 2014
View reviewed changes

JamesRTaylor reviewed Jul 12, 2014
View reviewed changes

asfgit closed this Aug 30, 2014

asfgit deleted the local-index branch August 30, 2014 15:55

elilevine mentioned this pull request Aug 11, 2015

PHOENIX-1673 Allow TenantId to be of any integral data type #104

Closed

priyankporwal mentioned this pull request May 1, 2019

PHOENIX-4703 Make indextool changes to drop before rebuild #495

Merged

tkhurana added a commit to tkhurana/phoenix that referenced this pull request Jan 15, 2023

Where optimizer poc apache#1

1871bfb

tkhurana added a commit to tkhurana/phoenix that referenced this pull request May 25, 2023

Where optimizer poc apache#1

f3cc159

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-933 Local index support to Phoenix #1

PHOENIX-933 Local index support to Phoenix #1

chrajeshbabu commented Jul 4, 2014

JamesRTaylor Jul 9, 2014

chrajeshbabu Jul 11, 2014

JamesRTaylor Jul 12, 2014

chrajeshbabu Jul 12, 2014

JamesRTaylor commented Jul 9, 2014

JamesRTaylor Jul 9, 2014

JamesRTaylor Jul 9, 2014

chrajeshbabu Jul 12, 2014

chrajeshbabu Jul 12, 2014

JamesRTaylor Jul 12, 2014

JamesRTaylor commented Jul 9, 2014

chrajeshbabu commented Jul 12, 2014

JamesRTaylor Jul 12, 2014

JamesRTaylor Jul 12, 2014

chrajeshbabu Jul 13, 2014

JamesRTaylor commented Jul 12, 2014

chrajeshbabu commented Jul 13, 2014

JamesRTaylor commented Jul 13, 2014

PHOENIX-933 Local index support to Phoenix #1

PHOENIX-933 Local index support to Phoenix #1

Conversation

chrajeshbabu commented Jul 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Jul 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Jul 9, 2014

chrajeshbabu commented Jul 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesRTaylor commented Jul 12, 2014

chrajeshbabu commented Jul 13, 2014

JamesRTaylor commented Jul 13, 2014