[GitHub] phoenix pull request #314: PHOENIX-4820
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/314#discussion_r206377190 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java --- @@ -559,8 +559,16 @@ protected QueryPlan compileSingleFlatQuery(StatementContext context, SelectState groupBy = groupBy.compile(context, innerPlanTupleProjector); context.setResolver(resolver); // recover resolver RowProjector projector = ProjectionCompiler.compile(context, select, groupBy, asSubquery ? Collections.emptyList() : targetColumns, where); -OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, limit, offset, projector, -groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, isInRowKeyOrder); +OrderBy orderBy = OrderByCompiler.compile( +context, +select, +groupBy, +limit, +offset, +projector, +groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, +groupBy == GroupBy.EMPTY_GROUP_BY ? isInRowKeyOrder : true, +where); --- End diff -- Eliminating order-by based on the inner plan ordering is definitely the right and ultimate solution, and we should get rid of all âdirty fixâ flags here. The compile result, in our case, the QueryPlan should have a definite ordering whether itâs from an order by, or a group-by, or a natural row order, or an order-by from the inner query. However, letting QueryPlan contain that information might not be a good idea. So we should see if we can infer the ordering from a QueryPlan. ---
[GitHub] phoenix pull request #278: PHOENIX-4322 DESC primary key column with variabl...
Github user maryannxue closed the pull request at: https://github.com/apache/phoenix/pull/278 ---
[GitHub] phoenix pull request #228: PHOENIX-3355 Register Phoenix built-in functions ...
Github user maryannxue closed the pull request at: https://github.com/apache/phoenix/pull/228 ---
[GitHub] phoenix pull request #281: PHOENIX-4288 Indexes not used when ordering by pr...
Github user maryannxue closed the pull request at: https://github.com/apache/phoenix/pull/281 ---
[GitHub] phoenix pull request #301: PHOENIX-4728 The upsert select must project tuple...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/301#discussion_r190428434
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java ---
@@ -549,7 +549,7 @@ public MutationPlan compile(UpsertStatement upsert)
throws SQLException {
select = SelectStatement.create(select, hint);
// Pass scan through if same table in upsert and select so
that projection is computed correctly
// Use optimizer to choose the best plan
-QueryCompiler compiler = new QueryCompiler(statement, select,
selectResolver, targetColumns, parallelIteratorFactoryToBe, new
SequenceManager(statement), false, false, null);
+QueryCompiler compiler = new QueryCompiler(statement, select,
selectResolver, targetColumns, parallelIteratorFactoryToBe, new
SequenceManager(statement), true, false, null);
--- End diff --
"Tuple projection" was first used for join queries so that columns are
accessed based on positions instead of names. We later applied this to
single-table queries, but for some reason (reason that I can't recall right
now), we wanted to avoid tuple projection in UPSERT. If this change won't cause
any existing test failure, I think it's just fine.
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183133375
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -314,7 +314,8 @@ protected QueryPlan
compileJoinQuery(JoinCompiler.Strategy strategy, StatementCo
if (i < count - 1) {
fieldPositions[i + 1] = fieldPositions[i] +
(tables[i] == null ? 0 : (tables[i].getColumns().size() -
tables[i].getPKColumns().size()));
}
-hashPlans[i] = new HashSubPlan(i, subPlans[i],
optimized ? null : hashExpressions, joinSpec.isSingleValueOnly(),
keyRangeLhsExpression, keyRangeRhsExpression);
+boolean usePersistentCache =
joinTable.getStatement().getHint().hasHint(Hint.USE_PERSISTENT_CACHE);
--- End diff --
We can make "usePersistentCache" a member of QueryCompiler and initialize
it in the beginning just like "noChildParentJoinOptimization".
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183127442
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/cache/ServerCacheClient.java ---
@@ -216,22 +234,146 @@ public void close() throws SQLException {
}
}
}
-
+}
+
+public ServerCache checkServerCache(final byte[] cacheId, ScanRanges
keyRanges, final TableRef cacheUsingTableRef,
--- End diff --
I am thinking here, can we do this differently?
Instead of making RPC calls of "checkServerCache" for the first and every
subsequent queries, we do NOT make any calls, neither "check" or "add" when the
persistent-cache hint is available and catches the
{{PersistentCacheNotFoundException}} on the first attempt (or later attempts if
somehow the cache has been evicted) and then try adding the cache all over
again. I think it will be more efficient in general.
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183130270
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/cache/TenantCacheImpl.java ---
@@ -77,57 +153,164 @@ public MemoryManager getMemoryManager() {
return memoryManager;
}
-private Cache<ImmutableBytesPtr,Closeable> getServerCaches() {
+private Cache<ImmutableBytesPtr,CacheEntry> getServerCaches() {
/* Delay creation of this map until it's needed */
if (serverCaches == null) {
synchronized(this) {
if (serverCaches == null) {
-serverCaches = CacheBuilder.newBuilder()
-.expireAfterAccess(maxTimeToLiveMs,
TimeUnit.MILLISECONDS)
-.ticker(getTicker())
-.removalListener(new
RemovalListener<ImmutableBytesPtr, Closeable>(){
-@Override
-public void
onRemoval(RemovalNotification<ImmutableBytesPtr, Closeable> notification) {
-
Closeables.closeAllQuietly(Collections.singletonList(notification.getValue()));
-}
-})
-.build();
+serverCaches = buildCache(maxTimeToLiveMs, false);
}
}
}
return serverCaches;
}
+
+private Cache<ImmutableBytesPtr,CacheEntry>
getPersistentServerCaches() {
+/* Delay creation of this map until it's needed */
+if (persistentServerCaches == null) {
+synchronized(this) {
+if (persistentServerCaches == null) {
+persistentServerCaches =
buildCache(maxPersistenceTimeToLiveMs, true);
+}
+}
+}
+return persistentServerCaches;
+}
+
+private Cache<ImmutableBytesPtr, CacheEntry> buildCache(final int ttl,
final boolean isPersistent) {
+return CacheBuilder.newBuilder()
+.expireAfterAccess(ttl, TimeUnit.MILLISECONDS)
+.ticker(getTicker())
+.removalListener(new RemovalListener<ImmutableBytesPtr,
CacheEntry>(){
+@Override
+public void
onRemoval(RemovalNotification<ImmutableBytesPtr, CacheEntry> notification) {
+ if (isPersistent ||
!notification.getValue().getUsePersistentCache()) {
+
Closeables.closeAllQuietly(Collections.singletonList(notification.getValue()));
+ }
+}
+})
+.build();
+}
-@Override
+private void evictInactiveEntries(long bytesNeeded) {
+CacheEntry[] entries =
getPersistentServerCaches().asMap().values().toArray(new CacheEntry[]{});
+Arrays.sort(entries);
+long available = this.getMemoryManager().getAvailableMemory();
+for (int i = 0; i < entries.length && available < bytesNeeded;
i++) {
+CacheEntry entry = entries[i];
+if (!entry.isLive()) {
+ getServerCaches().invalidate(entry.getCacheId());
+
getPersistentServerCaches().invalidate(entry.getCacheId());
+available = this.getMemoryManager().getAvailableMemory();
+}
+}
+}
+
+private CacheEntry maybeGet(ImmutableBytesPtr cacheId) {
+maybePromote(cacheId);
+CacheEntry entry = getServerCaches().getIfPresent(cacheId);
+return entry;
+}
+
+private void maybePromote(ImmutableBytesPtr cacheId) {
+CacheEntry entry =
getPersistentServerCaches().getIfPresent(cacheId);
+if (entry == null) {
+return;
+}
+getServerCaches().put(cacheId, entry);
+}
+
+private void maybeDemote(ImmutableBytesPtr cacheId) {
+CacheEntry entry = getServerCaches().getIfPresent(cacheId);
+if (entry == null) {
+return;
+}
+entry.decrementLiveQueryCount();
+if (!entry.isLive()) {
+getServerCaches().invalidate(cacheId);
+}
+}
+
+public void debugPrintCaches() {
+ System.out.println("Live cache:" + getServerCaches());
+ for (ImmutableBytesPtr key :
getServerCaches().asMap().keySet()) {
+ System.out.println("- " +
Hex.encodeHexString(key.get()) +
+ " -> " +
getServerCach
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183133702
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -383,7 +384,7 @@ protected QueryPlan
compileJoinQuery(JoinCompiler.Strategy strategy, StatementCo
new PTable[]{lhsTable}, new int[]{fieldPosition},
postJoinFilterExpression, QueryUtil.getOffsetLimit(limit, offset));
Pair<Expression, Expression> keyRangeExpressions = new
Pair<Expression, Expression>(null, null);
getKeyExpressionCombinations(keyRangeExpressions, context,
joinTable.getStatement(), rhsTableRef, type, joinExpressions, hashExpressions);
-return HashJoinPlan.create(joinTable.getStatement(),
rhsPlan, joinInfo, new HashSubPlan[]{new HashSubPlan(0, lhsPlan,
hashExpressions, false, keyRangeExpressions.getFirst(),
keyRangeExpressions.getSecond())});
+return HashJoinPlan.create(joinTable.getStatement(),
rhsPlan, joinInfo, new HashSubPlan[]{new HashSubPlan(0, lhsPlan,
hashExpressions, false, false, keyRangeExpressions.getFirst(),
keyRangeExpressions.getSecond())});
--- End diff --
This is also hash-join but with left table as build side. I think we should
be able to use persistent cache as well. And could you also add another test to
cover this point?
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183136068
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/memory/GlobalMemoryManager.java
---
@@ -159,6 +161,10 @@ protected void finalize() throws Throwable {
}
private void freeMemory() {
+// System.out.println("Free memory! " + size);
--- End diff --
I think this a mistake, missed from your code clean-up.
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183128249
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/cache/TenantCacheImpl.java ---
@@ -77,57 +153,164 @@ public MemoryManager getMemoryManager() {
return memoryManager;
}
-private Cache<ImmutableBytesPtr,Closeable> getServerCaches() {
+private Cache<ImmutableBytesPtr,CacheEntry> getServerCaches() {
/* Delay creation of this map until it's needed */
if (serverCaches == null) {
synchronized(this) {
if (serverCaches == null) {
-serverCaches = CacheBuilder.newBuilder()
-.expireAfterAccess(maxTimeToLiveMs,
TimeUnit.MILLISECONDS)
-.ticker(getTicker())
-.removalListener(new
RemovalListener<ImmutableBytesPtr, Closeable>(){
-@Override
-public void
onRemoval(RemovalNotification<ImmutableBytesPtr, Closeable> notification) {
-
Closeables.closeAllQuietly(Collections.singletonList(notification.getValue()));
-}
-})
-.build();
+serverCaches = buildCache(maxTimeToLiveMs, false);
}
}
}
return serverCaches;
}
+
+private Cache<ImmutableBytesPtr,CacheEntry>
getPersistentServerCaches() {
+/* Delay creation of this map until it's needed */
+if (persistentServerCaches == null) {
+synchronized(this) {
+if (persistentServerCaches == null) {
+persistentServerCaches =
buildCache(maxPersistenceTimeToLiveMs, true);
+}
+}
+}
+return persistentServerCaches;
+}
+
+private Cache<ImmutableBytesPtr, CacheEntry> buildCache(final int ttl,
final boolean isPersistent) {
+return CacheBuilder.newBuilder()
+.expireAfterAccess(ttl, TimeUnit.MILLISECONDS)
+.ticker(getTicker())
+.removalListener(new RemovalListener<ImmutableBytesPtr,
CacheEntry>(){
+@Override
+public void
onRemoval(RemovalNotification<ImmutableBytesPtr, CacheEntry> notification) {
+ if (isPersistent ||
!notification.getValue().getUsePersistentCache()) {
+
Closeables.closeAllQuietly(Collections.singletonList(notification.getValue()));
--- End diff --
Looks like the indentation is not right here. Can you perform a check
through the entire PR?
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183135634
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/HashJoinPlan.java ---
@@ -488,11 +502,20 @@ public ServerCache execute(HashJoinPlan parent)
throws SQLException {
if (hashExpressions != null) {
ResultIterator iterator = plan.iterator();
try {
-cache =
-parent.hashClient.addHashCache(ranges,
iterator,
+final byte[] cacheId;
+if (usePersistentCache) {
--- End diff --
Like I said in an earlier comment, if using persistent cache, we don't need
to call "check" or "add" cache here, instead we catch a specific Exception and
addServerCache at this point. Still, we might wanna return "null" if this is
persistent cache so later on the cache "close" method won't be called by the
HashJoinPlan clean-up routine.
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183128592
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/cache/TenantCacheImpl.java ---
@@ -77,57 +153,164 @@ public MemoryManager getMemoryManager() {
return memoryManager;
}
-private Cache<ImmutableBytesPtr,Closeable> getServerCaches() {
+private Cache<ImmutableBytesPtr,CacheEntry> getServerCaches() {
/* Delay creation of this map until it's needed */
if (serverCaches == null) {
synchronized(this) {
if (serverCaches == null) {
-serverCaches = CacheBuilder.newBuilder()
-.expireAfterAccess(maxTimeToLiveMs,
TimeUnit.MILLISECONDS)
-.ticker(getTicker())
-.removalListener(new
RemovalListener<ImmutableBytesPtr, Closeable>(){
-@Override
-public void
onRemoval(RemovalNotification<ImmutableBytesPtr, Closeable> notification) {
-
Closeables.closeAllQuietly(Collections.singletonList(notification.getValue()));
-}
-})
-.build();
+serverCaches = buildCache(maxTimeToLiveMs, false);
}
}
}
return serverCaches;
}
+
+private Cache<ImmutableBytesPtr,CacheEntry>
getPersistentServerCaches() {
+/* Delay creation of this map until it's needed */
+if (persistentServerCaches == null) {
+synchronized(this) {
+if (persistentServerCaches == null) {
+persistentServerCaches =
buildCache(maxPersistenceTimeToLiveMs, true);
+}
+}
+}
+return persistentServerCaches;
+}
+
+private Cache<ImmutableBytesPtr, CacheEntry> buildCache(final int ttl,
final boolean isPersistent) {
+return CacheBuilder.newBuilder()
+.expireAfterAccess(ttl, TimeUnit.MILLISECONDS)
+.ticker(getTicker())
+.removalListener(new RemovalListener<ImmutableBytesPtr,
CacheEntry>(){
+@Override
+public void
onRemoval(RemovalNotification<ImmutableBytesPtr, CacheEntry> notification) {
+ if (isPersistent ||
!notification.getValue().getUsePersistentCache()) {
+
Closeables.closeAllQuietly(Collections.singletonList(notification.getValue()));
+ }
+}
+})
+.build();
+}
-@Override
+private void evictInactiveEntries(long bytesNeeded) {
+CacheEntry[] entries =
getPersistentServerCaches().asMap().values().toArray(new CacheEntry[]{});
+Arrays.sort(entries);
+long available = this.getMemoryManager().getAvailableMemory();
+for (int i = 0; i < entries.length && available < bytesNeeded;
i++) {
+CacheEntry entry = entries[i];
+if (!entry.isLive()) {
+ getServerCaches().invalidate(entry.getCacheId());
+
getPersistentServerCaches().invalidate(entry.getCacheId());
+available = this.getMemoryManager().getAvailableMemory();
+}
+}
+}
+
+private CacheEntry maybeGet(ImmutableBytesPtr cacheId) {
--- End diff --
Think we can just call it "get" or "getIfPresent" and merge
{{maybePromote}} into this call.
---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r183126183
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/join/HashJoinPersistentCacheIT.java
---
@@ -0,0 +1,94 @@
+/*
+ * 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.end2end.join;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertEquals;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.util.Properties;
+
+import org.apache.phoenix.end2end.join.HashJoinCacheIT.InvalidateHashCache;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.SchemaUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+
+public class HashJoinPersistentCacheIT extends BaseJoinIT {
+
+@Override
+protected String getTableName(Connection conn, String virtualName)
throws Exception {
+String realName = super.getTableName(conn, virtualName);
+TestUtil.addCoprocessor(conn,
SchemaUtil.normalizeFullTableName(realName), InvalidateHashCache.class);
+return realName;
+}
+
+@Test
+public void testPersistentCache() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ Connection conn = DriverManager.getConnection(getUrl(), props);
+
+ createTestTable(getUrl(), "CREATE TABLE IF NOT EXISTS states (state
CHAR(2) NOT NULL, name VARCHAR NOT NULL CONSTRAINT my_pk PRIMARY KEY (state,
name))");
+ createTestTable(getUrl(), "CREATE TABLE IF NOT EXISTS cities (state
CHAR(2) NOT NULL, city VARCHAR NOT NULL, population BIGINT CONSTRAINT my_pk
PRIMARY KEY (state, city))");
+
+ conn.prepareStatement("UPSERT INTO states VALUES ('CA',
'California')").executeUpdate();
+ conn.prepareStatement("UPSERT INTO states VALUES ('AZ',
'Arizona')").executeUpdate();
+ conn.prepareStatement("UPSERT INTO cities VALUES ('CA', 'San
Francisco', 5)").executeUpdate();
+ conn.prepareStatement("UPSERT INTO cities VALUES ('CA',
'Sacramento', 3000)").executeUpdate();
+ conn.prepareStatement("UPSERT INTO cities VALUES ('AZ', 'Phoenix',
2)").executeUpdate();
+ conn.commit();
+
+ /* First, run query without using the persistent cache. This should
return
+ * different results after an UPSERT takes place.
+ */
+ ResultSet rs = conn.prepareStatement("SELECT SUM(population) FROM
states s JOIN cities c ON c.state = s.state").executeQuery();
+ rs.next();
+ int population1 = rs.getInt(1);
+
+ conn.prepareStatement("UPSERT INTO cities VALUES ('CA', 'Mt View',
1500)").executeUpdate();
+ conn.commit();
+ rs = conn.prepareStatement("SELECT SUM(population) FROM states s
JOIN cities c ON c.state = s.state").executeQuery();
+ rs.next();
+ int population2 = rs.getInt(1);
+
+ assertEquals(73000, population1);
+ assertEquals(74500, population2);
+
+ /* Second, run query using the persistent cache. This should return
the
+ * same results after an UPSERT takes place.
+ */
+ rs = conn.prepareStatement("SELECT /*+ USE_PERSISTENT_CACHE */
SUM(population) FROM states s JOIN cities c ON c.state =
s.state").executeQuery();
+ rs.next();
+ int population3 = rs.getInt(1);
+
+ conn.prepareStatement("UPSERT INTO cities VALUES ('CA', 'Palo Alto',
2000)").executeUpdate();
+ conn.commit();
+ rs = conn.prepareStatement("SELECT /*+ USE_PERSISTENT_CACHE */
SUM(population) FROM states s JOIN cities c ON c.state =
s.state").executeQuery();
+ rs.next();
+ int popu
[GitHub] phoenix pull request #281: PHOENIX-4288 Indexes not used when ordering by pr...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/281#discussion_r153404439
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/CostBasedDecisionIT.java ---
@@ -0,0 +1,176 @@
+package org.apache.phoenix.end2end;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.util.Map;
+import java.util.Properties;
+
+import org.apache.phoenix.query.BaseTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.QueryUtil;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.google.common.collect.Maps;
+
+public class CostBasedDecisionIT extends BaseTest {
--- End diff --
Thanks for pointing this out, @JamesRTaylor! Changed in my latest CI as
suggested.
---
[GitHub] phoenix pull request #281: PHOENIX-4288 Indexes not used when ordering by pr...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/281#discussion_r151829519
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java ---
@@ -91,8 +91,23 @@ public QueryPlan optimize(PhoenixStatement statement,
SelectStatement select, Co
}
public QueryPlan optimize(QueryPlan dataPlan, PhoenixStatement
statement, List targetColumns, ParallelIteratorFactory
parallelIteratorFactory) throws SQLException {
-Listplans = getApplicablePlans(dataPlan, statement,
targetColumns, parallelIteratorFactory, true);
-return plans.get(0);
+List plans = getApplicablePlans(dataPlan, statement,
targetColumns, parallelIteratorFactory, false);
+if (plans.size() == 1) {
+return plans.get(0);
+}
+
+// Get the best plan based on their costs. Costs will be ZERO if
stats are not
+// available, thus the first plan will be returned.
+Cost minCost = null;
+QueryPlan bestPlan = null;
+for (QueryPlan plan : plans) {
+Cost cost = plan.getCost();
+if (minCost == null || cost.compareTo(minCost) < 0) {
+minCost = cost;
+bestPlan = plan;
+}
+}
+return bestPlan;
--- End diff --
Thank you both for the input. I've change the logic for hinted plans as
well as added new tests. Please review again.
---
[GitHub] phoenix issue #281: PHOENIX-4288 Indexes not used when ordering by primary k...
Github user maryannxue commented on the issue: https://github.com/apache/phoenix/pull/281 @JamesRTaylor, I made all the changes suggested in your comments except one: "need more test cases" Thought I'd naturally go down this path as I start to tackle other issues under PHOENIX-4313. Please let me know if this PR is good enough to be pushed in, thanks! ---
[GitHub] phoenix pull request #281: PHOENIX-4288 Indexes not used when ordering by pr...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/281#discussion_r151580119
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/optimize/QueryOptimizer.java ---
@@ -91,8 +91,23 @@ public QueryPlan optimize(PhoenixStatement statement,
SelectStatement select, Co
}
public QueryPlan optimize(QueryPlan dataPlan, PhoenixStatement
statement, List targetColumns, ParallelIteratorFactory
parallelIteratorFactory) throws SQLException {
-Listplans = getApplicablePlans(dataPlan, statement,
targetColumns, parallelIteratorFactory, true);
-return plans.get(0);
+List plans = getApplicablePlans(dataPlan, statement,
targetColumns, parallelIteratorFactory, false);
+if (plans.size() == 1) {
+return plans.get(0);
+}
+
+// Get the best plan based on their costs. Costs will be ZERO if
stats are not
+// available, thus the first plan will be returned.
+Cost minCost = null;
+QueryPlan bestPlan = null;
+for (QueryPlan plan : plans) {
+Cost cost = plan.getCost();
+if (minCost == null || cost.compareTo(minCost) < 0) {
+minCost = cost;
+bestPlan = plan;
+}
+}
+return bestPlan;
--- End diff --
Thanks for the feedback, @katameru! We need to define a strategy how hints
work in the cost-based optimizer mode. Your input would be very much
appreciated!
Since this is the first issue in which I've brought "cost-based" in, the
idea is to plug-in the framework and pieces of code here and there needed to
make it work in general. We can see there's still a lot to do to refine this
framework, and as I dig further into other issues under PHOENIX-4313. Right now
I think adding a option to disable cost-based optimizer (pls see my latest ci)
should be enough.
---
[GitHub] phoenix issue #281: PHOENIX-4288 Indexes not used when ordering by primary k...
Github user maryannxue commented on the issue: https://github.com/apache/phoenix/pull/281 Thank you for the review, @JamesRTaylor! These are very good comments. I'll update the PR accordingly. ---
[GitHub] phoenix pull request #281: PHOENIX-4288 Indexes not used when ordering by pr...
GitHub user maryannxue opened a pull request: https://github.com/apache/phoenix/pull/281 PHOENIX-4288 Indexes not used when ordering by primary key 1. Add class Cost. 2. Add method getCost() in QueryPlan. 3. Let QueryOptimizer choose the best plan based on Cost; meanwhile if stats are not available the QueryOptimizer will keep the original behavior. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maryannxue/phoenix phoenix-4388 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/281.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 #281 commit f75c1b095683c153f8749107c5505304309dcb36 Author: maryannxue <maryann@gmail.com> Date: 2017-11-07T21:40:37Z PHOENIX-4288 Indexes not used when ordering by primary key ---
[GitHub] phoenix pull request #278: PHOENIX-4322 DESC primary key column with variabl...
GitHub user maryannxue opened a pull request: https://github.com/apache/phoenix/pull/278 PHOENIX-4322 DESC primary key column with variable length does not work in SkipScanFilter Changes: Avoid adding an extra trailing separator to the key You can merge this pull request into a Git repository by running: $ git pull https://github.com/maryannxue/phoenix phoenix-4322 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/278.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 #278 ---
[GitHub] phoenix pull request #7: PHOENIX-852 Optimize child/parent foreign key joins
Github user maryannxue closed the pull request at: https://github.com/apache/phoenix/pull/7 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #228: PHOENIX-3355 Register Phoenix built-in functions ...
GitHub user maryannxue opened a pull request: https://github.com/apache/phoenix/pull/228 PHOENIX-3355 Register Phoenix built-in functions as Calcite functions You can merge this pull request into a Git repository by running: $ git pull https://github.com/maryannxue/phoenix phoenix-3355 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/228.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 #228 commit bba69c94ae15e1dea7950bcddf95b72e39ba89df Author: maryannxue <maryann@gmail.com> Date: 2016-12-20T20:19:41Z PHOENIX-3355 Register Phoenix built-in functions as Calcite functions --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/224#discussion_r91163250
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/calcite/CalciteIT.java ---
@@ -1343,6 +1343,28 @@ public void initTable() throws Exception {
{7L, "05", "T5", "05", "S5"},
{8L, "06", "T6", "06", "S6"}})
.close();
+
--- End diff --
Thank you for the info, @lomoree! Very helpful! I was wondering if there
had been any check of that kind during conversion. Anyway, would you mind
removing that NO_SEQUENCE predicate and trying the changes I suggested below?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/224#discussion_r91152331
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/calcite/CalciteIT.java ---
@@ -1343,6 +1343,28 @@ public void initTable() throws Exception {
{7L, "05", "T5", "05", "S5"},
{8L, "06", "T6", "06", "S6"}})
.close();
+
--- End diff --
I remember you said you ran into a problem using PhoenixServerProject, but
it's PhoenixClientProject here again. Does it oscillate between the two? Could
you please try {{start(false, 1f)}} instead?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/224#discussion_r91155610
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixClientProject.java
---
@@ -73,13 +70,10 @@ public QueryPlan implement(PhoenixRelImplementor
implementor) {
QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel)
getInput());
implementor.popContext();
-
-PhoenixSequence sequence = CalciteUtils.findSequence(this);
-final SequenceManager seqManager = sequence == null ?
-null : new SequenceManager(new
PhoenixStatement(sequence.pc));
-implementor.setSequenceManager(seqManager);
-TupleProjector tupleProjector = project(implementor);
-if (seqManager != null) {
+
--- End diff --
This entire wrapping logic should live in PhoenixToEnumerableConverter
instead. Wrapping it with an iterator not a query plan would be better,
something like:
{code}
return new DelegateQueryPlan((QueryPlan) plan) {
@Override
public ResultIterator iterator() throws SQLException {
ResultIterator iterator =
iterator(DefaultParallelScanGrouper.getInstance());
if
(phoenixImplementor.getSequenceManager().getSequenceCount() > 0) {
iterator = new SequenceResultIterator(iterator,
phoenixImplementor.getSequenceManager());
}
return iterator;
}
@Override
public ExplainPlan getExplainPlan() throws SQLException {
{code}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #224: PHOENIX-3477 Support sequence arithmetic in Calci...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/224#discussion_r91152478
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1115,6 +1099,11 @@ public Void visitCall(RexCall call) {
|| call.getKind() == SqlKind.NEXT_VALUE)) {
sequenceValueCall = call;
}
+if (sequenceValueCall == null){
--- End diff --
Think we are good to remove this class, right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #222: PHOENIX-3355 Register Phoenix built-in functions as Calc...
Github user maryannxue commented on the issue: https://github.com/apache/phoenix/pull/222 2. We'll leave it then. I think the future solution can be either 1) add as built-in funcs in Calcite; or 2) add type derivation mechanism for UDFs in Calcite. 3. See "getAdjustedTime" in CalciteUtils then. You might need to do something similar somewhere, or this method is doing something wrong in the first place. Please go ahead with 1, 3 and 6, but feel free to leave out 3 if it doesn't work out easily for you. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix issue #222: PHOENIX-3355 Register Phoenix built-in functions as Calc...
Github user maryannxue commented on the issue: https://github.com/apache/phoenix/pull/222 No differentiation between the regex functions (String based and Byte based) -- Let's just add support for this one? Invert and MD5 Function don't work (phoenix doesn't supply the correct arguments in annotation) -- Can we make it right, or is it more complicated than that? If the latter, we can open another issue. Type matching doesn't work on input arguments (date/timestamp) -- It's probably a Calcite issue. Let's try repo it as a Calcite test case. Let me know if you need help working with Calcite unit tests. Timezone parameter doesn't work properly in ToDateFunction (basic cases work, but custom timezones do not act as expected) I probably know this one. Not sure if this is the issue though: the date/time/timestamp objects are local time (w/ timezone offset) in Calcite/Avatica, while they are UTC time on the Phoenix side. Array constructors are not supported (another issue entirely) -- Yes, another issue. We could open a JIRA listing those test failures blocking on this issue as a way to keep track of them. As to the code refinement, I think there are 4 (maybe more?) big chunks of code: createBuiltinFunctions(), evaluateReturnType(), overloadArguments() and registerBuiltinFunctions(). We should probably put all these in one place. I prefer in PhoenixSchema, otherwise CalciteUtils could also do. What makes sense to where the code should live, aside from what class of object it instantiates, is also who is the caller or who decides the logic or routines. So it seems to me that PhoenixScalarFunction here is only the "callee" class which is used by PhoenixSchema and CalciteUtils. There are a couple of things here that eventually forms the complete list of functions that we register in Calcite: 1. We create multiple signatures corresponding to each overload of the same function. 2. We merge all versions of the same signature but with different default parameter counts into one function. 3. We deal with aliases. Part of the reason why we are doing this is that we are trying to reorganize the existing function dictionary into something that would match the Calcite conventions. The implementation could end up being very different if the existing Phoenix function structure were something else. So I guess it'd be easier to maintain if we put these methods all in one place and maybe we should add more code comment describing what each of these methods do. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #223: PHOENIX-3488 Support COUNT(DISTINCT x) in Phoenix...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/223#discussion_r90119141
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/rules/PhoenixConverterRules.java
---
@@ -899,9 +899,6 @@ public static boolean isConvertible(Aggregate input) {
if (input.getGroupSets().size() > 1)
return false;
-if (input.containsDistinctCall())
--- End diff --
One minor nit, would you mind add check for other distinct functions? coz
we don't support SUM DISTINCT yet.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #222: PHOENIX-3355 Register Phoenix built-in functions ...
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/222#discussion_r87515184 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java --- @@ -127,6 +129,7 @@ private static void addBuiltInFunction(Class f) th } int nArgs = d.args().length; BuiltInFunctionInfo value = new BuiltInFunctionInfo(f, d); +SINGLE_SIGNATURE_BUILT_IN_FUNCTION_MAP.put(value.getName(), value); --- End diff -- We'll have to adjust the map initialization logic here according to the new annotation. Avoid "ABSTRACT" for "SINGLE_SIGNATURE_BUILT_IN_FUNCTION_MAP" and "DERIVED" for "BUILT_IN_FUNCTION_MAP". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #222: PHOENIX-3355 Register Phoenix built-in functions ...
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/222#discussion_r87512477 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/parse/FunctionParseNode.java --- @@ -30,6 +30,7 @@ import java.util.List; --- End diff -- Probably there will be no need for any change in this file other than adding one more annotation attribute and one more field in BuiltinFunctionInfo. Let's move the "overloadArguments()" to PhoenixSchema. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #222: PHOENIX-3355 Register Phoenix built-in functions ...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/222#discussion_r87512188
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/PhoenixScalarFunction.java
---
@@ -90,6 +153,10 @@ public PFunction getFunctionInfo() {
return functionInfo;
}
+public FunctionParseNode.BuiltInFunctionInfo getParseInfo(){
--- End diff --
So here maybe all we need is the exact FunctionExpression class. In case of
abstract function class, we need some additional information in
BuiltinFunctionInfo. So I'm thinking to add annotation for those derived
function expression classes, e.g. RoundDateFunction as well. And in order to
let the standalone Phoenix function map not to add the derived signatures and
meanwhile to make sure that calcite-phoenix's function map will not have the
abstract function classes, we can add one more attribute to BuiltinFunction as:
{code}
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public
@interface Argument {
Class[] allowedTypes() default {};
boolean isConstant() default false;
String defaultValue() default "";
String enumeration() default "";
String minValue() default "";
String maxValue() default "";
FunctionClassType classType() default FunctionClassType.NONE;
}
public enum FunctionClassType {
NONE,
ABSTRACT,
DERIVED,
}
{code}
Thus in CalciteUtils, we'll use reflection to get the function constructor,
either with or without StatementContext, depending on each individual function,
and instantiate the instance.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #222: PHOENIX-3355 Register Phoenix built-in functions ...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/222#discussion_r87515224
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/parse/ParseNodeFactory.java ---
@@ -181,6 +184,11 @@ public static BuiltInFunctionInfo get(String
normalizedName, List chi
return info;
}
+public static Collection getAll(){
--- End diff --
Give it a better name please :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #222: PHOENIX-3355 Register Phoenix built-in functions ...
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/222#discussion_r87512533 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/parse/PFunction.java --- @@ -18,14 +18,18 @@ package org.apache.phoenix.parse; --- End diff -- Please revert for this file. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #220: PHOENIX-3394 Handle SequenceResolving through Con...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/220#discussion_r86287165
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/PhoenixSchema.java ---
@@ -329,24 +333,14 @@ private TableRef fixTableMultiTenancy(TableRef
tableRef) throws SQLException {
private PhoenixSequence resolveSequence(String name) {
try {
-// FIXME: Do this the same way as resolving a table after
PHOENIX-2489.
-String tenantId = pc.getTenantId() == null ? null :
pc.getTenantId().getString();
-String q = "select 1 from " +
PhoenixDatabaseMetaData.SYSTEM_SEQUENCE
-+ " where " + PhoenixDatabaseMetaData.SEQUENCE_SCHEMA
-+ (schemaName == null ? " is null" : " = '" +
schemaName + "'")
-+ " and " + PhoenixDatabaseMetaData.SEQUENCE_NAME
-+ " = '" + name + "'"
-+ " and " + PhoenixDatabaseMetaData.TENANT_ID
-+ (tenantId == null ? " is null" : " = '" + tenantId +
"'");
-ResultSet rs = pc.createStatement().executeQuery(q);
-if (rs.next()) {
-return new PhoenixSequence(schemaName, name, pc);
-}
-} catch (SQLException e) {
-throw new RuntimeException(e);
+SequenceManager manager = new
SequenceManager((PhoenixStatement)pc.createStatement());
+manager.newSequenceReference(pc.getTenantId(),
TableName.createNormalized(schemaName, name) , null,
SequenceValueParseNode.Op.NEXT_VALUE);
--- End diff --
@JamesRTaylor Could you also take a look here to make sure that the method
is called with right parameters? And is there a chance (and a need) we can
avoid creating a new "Statement" each time?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #220: PHOENIX-3394 Handle SequenceResolving through Con...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/220#discussion_r86287255
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/calcite/CalciteIT.java ---
@@ -1315,6 +1315,17 @@ public void initTable() throws Exception {
{6L, "00A323122312312"},
{8L, "00A423122312312"}})
.close();
+
--- End diff --
It's nice to add more test case in CalciteIT, but I'm just curious, why
aren't existing tests in testSequence() sufficient?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #215: PHOENIX-3345 SQLException code's not propagating ...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/215#discussion_r82879380
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1115,4 +1115,15 @@ public static Object convertSqlLiteral(SqlLiteral
literal, PhoenixRelImplementor
+ " to its object type.", ex);
}
}
+
+public static SQLException unwrapSqlException(SQLException root){
+Exception e = root;
+while(e.getCause() != null){
+e = (Exception) e.getCause();
+if(e instanceof SQLException){
+root = (SQLException) e;
+}
+}
+return root;
+}
--- End diff --
Thanks for the modification, @lomoree! It think it's fine to go for the
deepest SQLException. But meanwhile I'm not sure if we should catch
RuntimeException as well and then either unwrap it if we can find an inner
SQLException or wrap it if not. @JamesRTaylor Any idea?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #215: PHOENIX-3345 SQLException code's not propagating ...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/215#discussion_r82686793
--- Diff:
phoenix-core/src/main/java/org/apache/calcite/jdbc/PhoenixCalciteFactory.java
---
@@ -161,6 +162,29 @@ public Object apply(
}));
}
+public CalciteStatement createStatement(String sql, int
resultSetType, int resultSetConcurrency, int resultSetHoldability) throws
SQLException {
+try {
+return super.createStatement(resultSetType,
resultSetConcurrency, resultSetHoldability);
+} catch (SQLException e) {
+if (e.getCause().getCause() instanceof SQLException) {
+throw (SQLException) e.getCause().getCause();
--- End diff --
Can we can have a utility method that unwraps the Exception? And as we
might different number of levels of Exception wrapping, can we just look down
one level at a time till we hit an instance of SQLException?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #214: PHOENIX-3298 Single column primary key may not be...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/214#discussion_r82285685
--- Diff:
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -1453,7 +1453,7 @@ public void testInvalidPrimaryKeyDecl() throws
Exception {
}
}
}
-
+
--- End diff --
@lomoree It's nice to avoid these trailing spaces, but most of the time, we
need to avoid such changes in our patch, especially for 'calcite' branch at the
moment, for it would make future merge to master more difficult.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #214: PHOENIX-3298 Single column primary key may not be null
Github user maryannxue commented on the issue: https://github.com/apache/phoenix/pull/214 Sure. I'll do that. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/213#discussion_r81430086
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws
SQLException {
}
@Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null &&
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null &&
limit.equals(this.limit))) {
--- End diff --
Is it a SQL-92 standard that OFFSET can only be used together with LIMIT? I
know there are some dialects that allow different OFFSET/LIMIT or OFFSET/FETCH
grammars, so I'm just confused... I tried with Calcite parser and it does allow
OFFSET without LIMIT.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/213#discussion_r81418958
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixLimit.java ---
@@ -76,20 +76,27 @@ public RelOptCost computeSelfCost(RelOptPlanner
planner, RelMetadataQuery mq) {
@Override
public double estimateRowCount(RelMetadataQuery mq) {
-double rows = super.estimateRowCount(mq);
+double rows = super.estimateRowCount(mq);
+if(offset != null) {
+return Math.max(0, Math.min(RexLiteral.intValue(fetch), rows -
RexLiteral.intValue(offset)));
+}
return Math.min(RexLiteral.intValue(fetch), rows);
}
@Override
public QueryPlan implement(PhoenixRelImplementor implementor) {
QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel)
getInput());
int fetchValue = RexLiteral.intValue(fetch);
+int offsetValue = 0;
+if (offset != null){
+offsetValue = RexLiteral.intValue(offset);
+}
if (plan.getLimit() == null) {
--- End diff --
should be "if (plan.getLimit() == null && plan.getOffset() == null)"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/213#discussion_r81419366
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixLimit.java ---
@@ -76,20 +76,27 @@ public RelOptCost computeSelfCost(RelOptPlanner
planner, RelMetadataQuery mq) {
@Override
public double estimateRowCount(RelMetadataQuery mq) {
-double rows = super.estimateRowCount(mq);
+double rows = super.estimateRowCount(mq);
+if(offset != null) {
+return Math.max(0, Math.min(RexLiteral.intValue(fetch), rows -
RexLiteral.intValue(offset)));
+}
return Math.min(RexLiteral.intValue(fetch), rows);
}
@Override
public QueryPlan implement(PhoenixRelImplementor implementor) {
QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel)
getInput());
int fetchValue = RexLiteral.intValue(fetch);
--- End diff --
I believe now that we've enabled offset, "this.fetch" could be also be
null, which means it should be handled the same way as this.offset now. Could
you please also add test cases to cover situations like "offset != null" but
"limit == null"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/213#discussion_r81420342
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientScanPlan.java ---
@@ -106,12 +106,18 @@ public ExplainPlan getExplainPlan() throws
SQLException {
}
@Override
-public QueryPlan limit(Integer limit) {
-if (limit == this.limit || (limit != null &&
limit.equals(this.limit)))
+public QueryPlan limit(Integer limit, Integer offset) {
+if (limit == this.limit || (limit != null &&
limit.equals(this.limit))) {
--- End diff --
I can see there's a big confusion here. Basically, offset and limit should
be treated the same way. So here, it should be "if both the old limit and the
old offset are same as as the new ones specified, we return this; otherwise we
create a new object."
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #213: PHOENIX-2827 Support OFFSET in Calcite-Phoenix
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/213#discussion_r81419580
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/rel/PhoenixLimit.java ---
@@ -76,20 +76,27 @@ public RelOptCost computeSelfCost(RelOptPlanner
planner, RelMetadataQuery mq) {
@Override
public double estimateRowCount(RelMetadataQuery mq) {
-double rows = super.estimateRowCount(mq);
+double rows = super.estimateRowCount(mq);
+if(offset != null) {
+return Math.max(0, Math.min(RexLiteral.intValue(fetch), rows -
RexLiteral.intValue(offset)));
+}
return Math.min(RexLiteral.intValue(fetch), rows);
}
@Override
public QueryPlan implement(PhoenixRelImplementor implementor) {
QueryPlan plan = implementor.visitInput(0, (PhoenixQueryRel)
getInput());
int fetchValue = RexLiteral.intValue(fetch);
+int offsetValue = 0;
+if (offset != null){
+offsetValue = RexLiteral.intValue(offset);
+}
if (plan.getLimit() == null) {
-return plan.limit(fetchValue);
+return plan.limit(fetchValue, offsetValue);
}
return new ClientScanPlan(plan.getContext(), plan.getStatement(),
implementor.getTableMapping().getTableRef(),
RowProjector.EMPTY_PROJECTOR,
-fetchValue, null, null, OrderBy.EMPTY_ORDER_BY, plan);
+fetchValue, offsetValue, null, OrderBy.EMPTY_ORDER_BY,
plan);
--- End diff --
Since either of them could be null, Boolean objects should be used instead
of primitive boolean type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81214035
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
--- End diff --
Thank you, Eric, for doing a nice job and trying to work things out in the
best you could find! But if you run into issues like this next time, you can
just ping me quickly. I can probably give you some useful information since
I've had quite some experience dealing with Calcite.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81212867
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
+SqlNodeList.EMPTY, null, null, null).toString();
+SqlNode sqlNode = planner.parse(sql);
+sqlNode = planner.validate(sqlNode);
+Project proj = (Project) (planner.rel(sqlNode).rel);
+RexNode rex = proj.getChildExps().get(0);
+
+Expression e = CalciteUtils.toExpression(rex, implementor);
+ImmutableBytesWritable ptr = new ImmutableBytesWritable();
+e = ExpressionUtil.getConstantExpression(e, ptr);
+Object ret = e.getDataType().toObject(ptr);
+if(ret instanceof NlsString){
--- End diff --
I was talking about the "if" block, the NlsString is a Calcite thing, so
once we have converted a RexNode into an Phoenix Expression, there should only
be Java String objects. So you should probably just remove it and verify with a
test case (maybe already covered by an existing test case).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81211365
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
--- End diff --
Maybe let's leave it here in CalciteUtils if that works. They don't
necessarily belong anywhere, and CalciteUtils seems to be a class best for
hiding all the details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81210515
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
--- End diff --
Ok, I once ran into this as well. Let's just do string then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81200946
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
--- End diff --
Other convertXXX methods are to convert RexNode to Expression, so I'm
thinking something like "convertSqlLiteral(...)" could be less confusing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81201783
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
+SqlNodeList.EMPTY, null, null, null).toString();
+SqlNode sqlNode = planner.parse(sql);
+sqlNode = planner.validate(sqlNode);
+Project proj = (Project) (planner.rel(sqlNode).rel);
--- End diff --
Probably better to add an "assert" here to ensure the rel object is of
Project class before casting it. Similarly, assert that this Project has only
one expression for the following statement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81202889
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
+SqlNodeList.EMPTY, null, null, null).toString();
+SqlNode sqlNode = planner.parse(sql);
+sqlNode = planner.validate(sqlNode);
+Project proj = (Project) (planner.rel(sqlNode).rel);
+RexNode rex = proj.getChildExps().get(0);
+
+Expression e = CalciteUtils.toExpression(rex, implementor);
+ImmutableBytesWritable ptr = new ImmutableBytesWritable();
+e = ExpressionUtil.getConstantExpression(e, ptr);
+Object ret = e.getDataType().toObject(ptr);
+if(ret instanceof NlsString){
+ret = ((NlsString) ret).toString();
+}
+return ret;
+}
+catch (Exception e){
+throw new RuntimeException("Could not convert literal to its
object type: " + e);
--- End diff --
Normally you don't append the Exception itself to the end of the error
message. You message should include the exact SqlLiteral it failed to convert
and add the original Exception as inner exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81202299
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
+SqlNodeList.EMPTY, null, null, null).toString();
+SqlNode sqlNode = planner.parse(sql);
+sqlNode = planner.validate(sqlNode);
+Project proj = (Project) (planner.rel(sqlNode).rel);
+RexNode rex = proj.getChildExps().get(0);
+
+Expression e = CalciteUtils.toExpression(rex, implementor);
+ImmutableBytesWritable ptr = new ImmutableBytesWritable();
+e = ExpressionUtil.getConstantExpression(e, ptr);
+Object ret = e.getDataType().toObject(ptr);
+if(ret instanceof NlsString){
--- End diff --
Is this step necessary? I suppose no. But if yes,
CalciteUtils.toExpression() must have not done its job correctly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81201327
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
+
+SqlParserPos POS = SqlParserPos.ZERO;
+final SqlNodeList selectList =
+new SqlNodeList(
+Collections.singletonList(literal),
+SqlParserPos.ZERO);
+
+
+String sql = new SqlSelect(POS, SqlNodeList.EMPTY, selectList,
null, null, null, null,
--- End diff --
Can we skip making a sql string and parsing again, coz you already have a
SqlSelect node? We can go directly to planner.validate() and planner.rel(),
right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81203125
--- Diff:
phoenix-core/src/main/java/org/apache/calcite/sql/SqlOptionNode.java ---
@@ -38,12 +41,10 @@ public SqlOptionNode(SqlParserPos pos, SqlIdentifier
key, SqlLiteral literal) {
familyName = key.names.get(0);
propertyName = key.names.get(1);
}
-final Object v = SqlLiteral.value(literal);
-if (v instanceof NlsString) {
-value = ((NlsString) v).toString();
-} else {
-value = v;
-}
+
+PhoenixRelImplementor
+implementor = new PhoenixRelImplementorImpl(new
RuntimeContextImpl());
--- End diff --
Looks like we need a "dummy" RuntimeContext for this kind of "dry run"
implementor. I'll do this part when I commit it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #212: PHOENIX-3264 Allow TRUE and FALSE to be used as l...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/212#discussion_r81200246
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/calcite/CalciteUtils.java ---
@@ -1036,4 +1048,40 @@ public Void visitCall(RexCall call) {
return null;
}
}
+
+public static Object convertLiteral(SqlLiteral literal,
PhoenixRelImplementor implementor) {
+try {
+final SchemaPlus rootSchema =
Frameworks.createRootSchema(true);
+final FrameworkConfig config = Frameworks.newConfigBuilder()
+.parserConfig(SqlParser.Config.DEFAULT)
+.defaultSchema(rootSchema).build();
+Planner planner = Frameworks.getPlanner(config);
--- End diff --
Since this method will be called once for each SQLLiteral, it might be
better make these objects static members of CalciteUtils if possible.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #175: PHOENIX-2405 Not for merge, just request a review...
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/175#discussion_r69668752 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/SortMergeJoinPlan.java --- @@ -45,10 +45,7 @@ import org.apache.phoenix.exception.SQLExceptionInfo; import org.apache.phoenix.execute.TupleProjector.ProjectedValueTuple; import org.apache.phoenix.expression.Expression; -import org.apache.phoenix.iterate.DefaultParallelScanGrouper; -import org.apache.phoenix.iterate.MappedByteBufferQueue; -import org.apache.phoenix.iterate.ParallelScanGrouper; -import org.apache.phoenix.iterate.ResultIterator; +import org.apache.phoenix.iterate.*; --- End diff -- Please do always check your patch before you submit. This is not the right coding style, and such changes should never appear in a patch/pull request. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #175: PHOENIX-2405 Not for merge, just request a review...
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/175#discussion_r69668528 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/AggregatePlan.java --- @@ -133,7 +134,10 @@ public PeekingResultIterator newIterator(StatementContext context, ResultIterato Expression expression = RowKeyExpression.INSTANCE; OrderByExpression orderByExpression = new OrderByExpression(expression, false, true); int threshold = services.getProps().getInt(QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB, QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES); -return new OrderedResultIterator(scanner, Collections.singletonList(orderByExpression), threshold); +String spoolDirectory = services.getProps().get(QueryServices.SPOOL_DIRECTORY, QueryServicesOptions.DEFAULT_SPOOL_DIRECTORY); --- End diff -- In my previous review I might not have made it very clear that the idea of moving MemoryManager into constructors was more of a question rather than advice. Since I hadn't studied the code so carefully, I was trying to ask you if that would be better, or worse? Looks like these parameters are getting all over the place and my suggestion was not a good one? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request #175: PHOENIX-2405 Not for merge, just request a review...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/175#discussion_r69668052
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/SpoolingByteBufferSegmentQueue.java
---
@@ -0,0 +1,357 @@
+/*
+ * 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 org.apache.commons.io.input.CountingInputStream;
+import org.apache.commons.io.output.DeferredFileOutputStream;
+
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+
+
+import java.io.*;
+import java.util.AbstractQueue;
+import java.util.Iterator;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.apache.phoenix.monitoring.GlobalClientMetrics.*;
+
+
+public abstract class SpoolingByteBufferSegmentQueue extends
AbstractQueue {
+
+private ResultQueue spoolFrom;
+
+private boolean closed ;
+private boolean flushed;
+private DeferredFileOutputStream spoolTo;
+private MemoryChunk chunk;
+private int size = 0;
+private long inMemByteSize = 0L;
+private int index;
+
+
+
+SpoolingByteBufferSegmentQueue(int index, MemoryManager mm, final int
thresholdBytes, String spoolDirectory) {
+
+long startTime = System.currentTimeMillis();
+chunk = mm.allocate(0, thresholdBytes);
+long waitTime = System.currentTimeMillis() - startTime;
+GLOBAL_MEMORY_WAIT_TIME.update(waitTime);
+
+int size = (int)chunk.getSize();
+spoolTo = new DeferredFileOutputStream(size,
"ResultSpooler",".bin", new File(spoolDirectory)) {
+@Override
+protected void thresholdReached() throws IOException {
+try {
+super.thresholdReached();
+} finally {
+chunk.close();
+}
+}
+};
+
+
+}
+
+public int index() {
+return this.index;
+}
+
+
+
+protected abstract InMemoryResultQueue
createInMemoryResultQueue(byte[] bytes, MemoryChunk memoryChunk);
+
+protected abstract OnDiskResultQueue createOnDiskResultQueue(File
file);
+
+@Override
+public boolean offer(T t) {
+if (closed || flushed){
+return false;
+}
+boolean result = writeRecord(t, spoolTo);
+if(result){
--- End diff --
I'm pretty lost at why this "if" block here, means we flush to disk all the
time? I'll stop here though, leaving all other doubts, coz this all (XXXQueue
classes) is gonna take a rewrite I think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #175: PHOENIX-2405 Not for merge, just request a review...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/175#discussion_r69668023
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/SpoolingByteBufferSegmentQueue.java
---
@@ -0,0 +1,357 @@
+/*
+ * 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 org.apache.commons.io.input.CountingInputStream;
+import org.apache.commons.io.output.DeferredFileOutputStream;
+
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+
+
+import java.io.*;
+import java.util.AbstractQueue;
+import java.util.Iterator;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.apache.phoenix.monitoring.GlobalClientMetrics.*;
+
+
+public abstract class SpoolingByteBufferSegmentQueue extends
AbstractQueue {
+
+private ResultQueue spoolFrom;
+
+private boolean closed ;
+private boolean flushed;
+private DeferredFileOutputStream spoolTo;
+private MemoryChunk chunk;
+private int size = 0;
+private long inMemByteSize = 0L;
+private int index;
+
+
+
+SpoolingByteBufferSegmentQueue(int index, MemoryManager mm, final int
thresholdBytes, String spoolDirectory) {
+
+long startTime = System.currentTimeMillis();
+chunk = mm.allocate(0, thresholdBytes);
+long waitTime = System.currentTimeMillis() - startTime;
+GLOBAL_MEMORY_WAIT_TIME.update(waitTime);
+
+int size = (int)chunk.getSize();
+spoolTo = new DeferredFileOutputStream(size,
"ResultSpooler",".bin", new File(spoolDirectory)) {
+@Override
+protected void thresholdReached() throws IOException {
+try {
+super.thresholdReached();
+} finally {
+chunk.close();
+}
+}
+};
+
+
+}
+
+public int index() {
+return this.index;
+}
+
+
+
+protected abstract InMemoryResultQueue
createInMemoryResultQueue(byte[] bytes, MemoryChunk memoryChunk);
+
+protected abstract OnDiskResultQueue createOnDiskResultQueue(File
file);
+
+@Override
+public boolean offer(T t) {
+if (closed || flushed){
+return false;
+}
+boolean result = writeRecord(t, spoolTo);
+if(result){
+if(!spoolTo.isInMemory()){
+flushToDisk();
+}
+size++;
+}
+
+
+return result;
+}
+
+protected abstract boolean writeRecord(T t, OutputStream outputStream);
+
+private void flushToMemory(){
+byte[] data = spoolTo.getData();
+chunk.resize(data.length);
+spoolFrom = createInMemoryResultQueue(data, chunk);
+GLOBAL_MEMORY_CHUNK_BYTES.update(data.length);
+flushed = true;
+}
+
+
+private void flushToDisk(){
+long sizeOfSpoolFile = spoolTo.getFile().length();
+GLOBAL_SPOOL_FILE_SIZE.update(sizeOfSpoolFile);
+GLOBAL_SPOOL_FILE_COUNTER.increment();
+spoolFrom = createOnDiskResultQueue(spoolTo.getFile());
+if (spoolTo.getFile() != null) {
+spoolTo.getFile().deleteOnExit();
+}
+inMemByteSize = 0;
+flushed = true;
+}
+
+
+public boolean isFlushed(){
+return flushed;
+}
+
+public T peek() {
+if(!flushed){
+flushToMemory();
+}
+return spoolFrom.peek();
+}
+
+@Override
+pu
[GitHub] phoenix pull request #175: PHOENIX-2405 Not for merge, just request a review...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/175#discussion_r69667743
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/SpoolingByteBufferSegmentQueue.java
---
@@ -0,0 +1,357 @@
+/*
+ * 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 org.apache.commons.io.input.CountingInputStream;
+import org.apache.commons.io.output.DeferredFileOutputStream;
+
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+
+
+import java.io.*;
+import java.util.AbstractQueue;
+import java.util.Iterator;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import static org.apache.phoenix.monitoring.GlobalClientMetrics.*;
+
+
+public abstract class SpoolingByteBufferSegmentQueue extends
AbstractQueue {
--- End diff --
The logic is very confusing here. My idea was to extend or modify the
current SpoolingResultIterator so that it can take a ResultEntry and/or a Tuple
as a record. But meanwhile this does not have to do with the XXXQueue here.
XXXQueue deals with the priority queue logic and SpoolingXXX deals with the
deferred byte buffer logic. Let me know whether you understand how it's
supposed to work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/171#discussion_r65294499
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/SortMergeJoinPlan.java ---
@@ -572,11 +569,11 @@ public MappedByteBufferTupleQueue(int thresholdBytes)
{
}
@Override
-protected Comparator<MappedByteBufferSegmentQueue>
getSegmentQueueComparator() {
-return new Comparator<MappedByteBufferSegmentQueue>() {
+protected Comparator<BufferSegmentQueue>
getSegmentQueueComparator() {
+return new Comparator<BufferSegmentQueue>() {
@Override
-public int compare(MappedByteBufferSegmentQueue q1,
-MappedByteBufferSegmentQueue q2) {
+public int compare(BufferSegmentQueue q1,
+ BufferSegmentQueue q2) {
return q1.index() - q2.index();
--- End diff --
Think we should eventually get rid of MappedByteBufferSegmentQueue and used
the DeferredXXX version instead. Maybe that'll also simply the level of
abstraction here. Go with the other changes first and see what you can do here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/171#discussion_r65294376
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/DeferredByteBufferSegmentQueue.java
---
@@ -0,0 +1,123 @@
+package org.apache.phoenix.iterate;
+
+import org.apache.commons.io.output.DeferredFileOutputStream;
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+
+import java.io.*;
+import java.util.*;
+
+public abstract class DeferredByteBufferSegmentQueue extends
BufferSegmentQueue {
+
+final MemoryChunk chunk;
+
+public DeferredByteBufferSegmentQueue(int index, int thresholdBytes,
+ boolean hasMaxQueueSize,
MemoryManager memoryManager) {
+super(index, thresholdBytes, hasMaxQueueSize);
+chunk = memoryManager.allocate(thresholdBytes);
+}
+
+abstract protected void writeToBuffer(OutputStream outputStream, T e);
+abstract protected T readFromBuffer(DataInput dataInput);
+
+
+@Override
+protected SegmentQueueFileIterator
createSegmentQueueFileIterator(SegmentQueueFileIterator iterator){
+return new DeferredSegmentQueueFileIterator(iterator);
+}
+
+@Override
+protected SegmentQueueFileIterator createSegmentQueueFileIterator(){
+return new DeferredSegmentQueueFileIterator();
+}
+
+@Override
+protected void flush(T entry) throws IOException {
+Queue inMemQueue = getInMemoryQueue();
+int resultSize = sizeOf(entry);
+maxResultSize = Math.max(maxResultSize, resultSize);
+totalResultSize = hasMaxQueueSize ? maxResultSize *
inMemQueue.size() : (totalResultSize + resultSize);
+if (totalResultSize >= thresholdBytes) {
+this.file = File.createTempFile(UUID.randomUUID().toString(),
null);
+
+DeferredFileOutputStream spoolTo = new
DeferredFileOutputStream(thresholdBytes, file) {
+@Override
+protected void thresholdReached() throws IOException {
+try {
+super.thresholdReached();
+} finally {
+chunk.close();
--- End diff --
There's an indent problem here. You might not need these lines after the
suggested change, but would you mind double checking other places for code
indent?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/171#discussion_r65294053
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/BufferSegmentQueue.java
---
@@ -0,0 +1,251 @@
+package org.apache.phoenix.iterate;
+
+import com.google.common.collect.Lists;
+import org.apache.commons.io.output.DeferredFileOutputStream;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.schema.tuple.Tuple;
+
+import java.io.*;
+import java.util.*;
+
+public abstract class BufferSegmentQueue extends AbstractQueue {
+protected static final int EOF = -1;
+
+protected final int index;
+protected final int thresholdBytes;
+protected final boolean hasMaxQueueSize;
+protected long totalResultSize = 0;
+protected int maxResultSize = 0;
+
+protected File file;
+private boolean isClosed = false;
+protected boolean flushBuffer = false;
+protected int flushedCount = 0;
+
+private T current = null;
+
+protected SegmentQueueFileIterator thisIterator;
+// iterators to close on close()
+protected List iterators;
+
+public BufferSegmentQueue(int index, int thresholdBytes, boolean
hasMaxQueueSize) {
+this.index = index;
+this.thresholdBytes = thresholdBytes;
+this.hasMaxQueueSize = hasMaxQueueSize;
+this.iterators = Lists. newArrayList();
+}
+
+abstract protected Queue getInMemoryQueue();
+abstract protected int sizeOf(T e);
+
+
+public int index() {
+return this.index;
+}
+
+public int size() {
+if (flushBuffer)
+return flushedCount;
+return getInMemoryQueue().size();
+}
+
+public long getInMemByteSize() {
+if (flushBuffer)
+return 0;
+return totalResultSize;
+}
+
+public boolean isFlushed() {
+return flushBuffer;
+}
+
+@Override
+public boolean offer(T e) {
+if (isClosed || flushBuffer)
+return false;
+
+boolean added = getInMemoryQueue().add(e);
+if (added) {
+try {
+flush(e);
+} catch (IOException ex) {
+throw new RuntimeException(ex);
+}
+}
+
+return added;
+}
+
+@Override
+public T peek() {
+if (current == null && !isClosed) {
+current = next();
+}
+
+return current;
+}
+
+@Override
+public T poll() {
+T ret = peek();
+if (!isClosed) {
+current = next();
+} else {
+current = null;
+}
+
+return ret;
+}
+
+@Override
+public Iterator iterator() {
+if (isClosed)
+return null;
+
+if (!flushBuffer)
+return getInMemoryQueue().iterator();
+
+SegmentQueueFileIterator iterator =
createSegmentQueueFileIterator(thisIterator);
--- End diff --
Just realized this existing implementation could be very inefficient, by
creating a new InputStream and skip a few bytes every time this is called. Is
it possible to always return the same iterator with the current read state?
Guess it will just work after using the SpoolingResultIterator logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/171#discussion_r65293621
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/DeferredByteBufferSegmentQueue.java
---
@@ -0,0 +1,123 @@
+package org.apache.phoenix.iterate;
+
+import org.apache.commons.io.output.DeferredFileOutputStream;
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+
+import java.io.*;
+import java.util.*;
+
+public abstract class DeferredByteBufferSegmentQueue extends
BufferSegmentQueue {
+
+final MemoryChunk chunk;
+
+public DeferredByteBufferSegmentQueue(int index, int thresholdBytes,
+ boolean hasMaxQueueSize,
MemoryManager memoryManager) {
+super(index, thresholdBytes, hasMaxQueueSize);
+chunk = memoryManager.allocate(thresholdBytes);
+}
+
+abstract protected void writeToBuffer(OutputStream outputStream, T e);
+abstract protected T readFromBuffer(DataInput dataInput);
--- End diff --
I know they are the existing method names, but they might be even clearer
if changed to "writeRecord(ToBuffer)" and "readRecord(FromBuffer)"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/171#discussion_r65293343
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/DeferredByteBufferSegmentQueue.java
---
@@ -0,0 +1,123 @@
+package org.apache.phoenix.iterate;
+
+import org.apache.commons.io.output.DeferredFileOutputStream;
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+
+import java.io.*;
+import java.util.*;
+
+public abstract class DeferredByteBufferSegmentQueue extends
BufferSegmentQueue {
+
+final MemoryChunk chunk;
+
+public DeferredByteBufferSegmentQueue(int index, int thresholdBytes,
+ boolean hasMaxQueueSize,
MemoryManager memoryManager) {
+super(index, thresholdBytes, hasMaxQueueSize);
+chunk = memoryManager.allocate(thresholdBytes);
+}
+
+abstract protected void writeToBuffer(OutputStream outputStream, T e);
+abstract protected T readFromBuffer(DataInput dataInput);
+
+
+@Override
+protected SegmentQueueFileIterator
createSegmentQueueFileIterator(SegmentQueueFileIterator iterator){
+return new DeferredSegmentQueueFileIterator(iterator);
+}
+
+@Override
+protected SegmentQueueFileIterator createSegmentQueueFileIterator(){
+return new DeferredSegmentQueueFileIterator();
+}
+
+@Override
+protected void flush(T entry) throws IOException {
+Queue inMemQueue = getInMemoryQueue();
+int resultSize = sizeOf(entry);
+maxResultSize = Math.max(maxResultSize, resultSize);
+totalResultSize = hasMaxQueueSize ? maxResultSize *
inMemQueue.size() : (totalResultSize + resultSize);
+if (totalResultSize >= thresholdBytes) {
+this.file = File.createTempFile(UUID.randomUUID().toString(),
null);
+
+DeferredFileOutputStream spoolTo = new
DeferredFileOutputStream(thresholdBytes, file) {
+@Override
+protected void thresholdReached() throws IOException {
+try {
+super.thresholdReached();
+} finally {
+chunk.close();
+}
+}
+};
+
+int resSize = inMemQueue.size();
+for (int i = 0; i < resSize; i++) {
+writeToBuffer(spoolTo, inMemQueue.poll());
+}
+
+spoolTo.write(EOF); // end
+spoolTo.flush();
+flushedCount = resSize;
+inMemQueue.clear();
+flushBuffer = true;
+}
+}
+
+private class DeferredSegmentQueueFileIterator extends
SegmentQueueFileIterator {
+private DataInputStream dataInput;
+
+public DeferredSegmentQueueFileIterator() {
+super();
+}
+
+public DeferredSegmentQueueFileIterator(SegmentQueueFileIterator
iterator) {
+super(iterator);
+}
+
+
+
+@Override
+protected void init(long readIndex) {
+this.isEnd = false;
+this.readIndex = readIndex;
+this.next = null;
+
+try {
+BufferedInputStream bufferedInputStream = new
BufferedInputStream(new FileInputStream(file));
+bufferedInputStream.skip(readIndex);
+this.dataInput = new DataInputStream(bufferedInputStream);
+} catch (IOException e) {
--- End diff --
Like I said in the above comment, you are not using
DeferredFileOutputStream as a deferred stream, you are just using it as an
ordinary FileOutputStream. So maybe forget about handling the
DeferredFileOutputStream yourself and use the adapted SpoolingResultIterator
instead.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/171#discussion_r65293136
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/DeferredByteBufferSegmentQueue.java
---
@@ -0,0 +1,123 @@
+package org.apache.phoenix.iterate;
+
+import org.apache.commons.io.output.DeferredFileOutputStream;
+import org.apache.phoenix.memory.MemoryManager;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+
+import java.io.*;
+import java.util.*;
+
+public abstract class DeferredByteBufferSegmentQueue extends
BufferSegmentQueue {
+
+final MemoryChunk chunk;
+
+public DeferredByteBufferSegmentQueue(int index, int thresholdBytes,
+ boolean hasMaxQueueSize,
MemoryManager memoryManager) {
+super(index, thresholdBytes, hasMaxQueueSize);
+chunk = memoryManager.allocate(thresholdBytes);
--- End diff --
"thresholdBytes" might be confusing here. There are actually two
occurrences of memory usage here, first one being in-memory priority queue for
sorting, once that part, the size of which is rather an estimate (based on the
priority queue data structure) than an actual value, has reached the threshold,
the priority queue content should be written to a some kind of file
OutputStream, which is now DeferredFileOutputStream. The second memory usage is
that used by DeferredFileOutputStream itself, since its content will first stay
in memory before its own threshold is reached.
Therefore, we might need to allocate twice (it's not real allocate anyway,
it's for tracking memory usage actually). But a better way to do this is to
make use of SpoolingResultIterator logic to handle the entire second part as
mentioned above. They should be exactly the same logic except that
SpoolingResultIterator writes and reads Tuples and what you need here is
something that writes and reads ResultEntry. So see if you can apply some
abstraction here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-2405
Github user maryannxue commented on a diff in the pull request: https://github.com/apache/phoenix/pull/171#discussion_r65291859 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/ScanRegionObserver.java --- @@ -234,6 +234,12 @@ protected RegionScanner doPostScannerOpen(final ObserverContext
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
Github user maryannxue commented on the pull request: https://github.com/apache/phoenix/pull/154#issuecomment-203722854 @ankitsinghal Some (maybe final) comments added. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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() ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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)". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2722 support mysql limit,offset clau...
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 {
+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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2257 Fix broken integration test for...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/120#discussion_r42860245
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java ---
@@ -313,21 +290,25 @@ public void testDerivedTableWithGroupBy() throws
Exception {
query = "SELECT DISTINCT COLLECTDISTINCT(t.b) FROM (SELECT
b_string b, a_string a FROM aTable GROUP BY a_string, b_string) AS t GROUP BY
t.a";
--- End diff --
@JamesRTaylor Please ignore my previous (deleted) comment. I just realized
that the server group-by has replaced tree map with hash map, which means an
optimization I did for client group-by based on the assumption that the input
is a sorted group-by is now invalid. I will fix this ASAP. And after the fix,
the test case should fine, we should keep it as it is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-2257 Fix broken integration test for...
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/120#discussion_r42859436
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/DerivedTableIT.java ---
@@ -313,21 +290,25 @@ public void testDerivedTableWithGroupBy() throws
Exception {
query = "SELECT DISTINCT COLLECTDISTINCT(t.b) FROM (SELECT
b_string b, a_string a FROM aTable GROUP BY a_string, b_string) AS t GROUP BY
t.a";
--- End diff --
@JamesRTaylor Do we have a deterministic ordering for arrays? If we do, I
don't think this change should be made without finding out the cause.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix-1580 union all impl
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/39#discussion_r25923422
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -147,21 +169,146 @@ public QueryCompiler(PhoenixStatement statement,
SelectStatement select, ColumnR
*/
public QueryPlan compile() throws SQLException{
SelectStatement select = this.select;
-ListObject binds = statement.getParameters();
+QueryPlan plan;
+if (isUnionAll()) {
+plan = compileUnionAll(select);
+} else {
+plan = compileSelect(select);
+}
+return plan;
+}
+
+public QueryPlan compileSelect(SelectStatement select) throws
SQLException{
+ // SelectStatement select = this.select;
+ ListObject binds = statement.getParameters();
+ StatementContext context = new StatementContext(statement,
resolver, scan, sequenceManager);
+ ColumnResolver resolver;
+ if (isUnionAll()) {
+ resolver = FromCompiler.getResolverForQuery(select,
statement.getConnection());
+ context = new StatementContext(statement, resolver, new
Scan(), sequenceManager);
+ }
+ if (select.isJoin()) {
+ if (isUnionAll()) {
+ resolver =
FromCompiler.getResolverForQuery(select, statement.getConnection());
+ select = JoinCompiler.optimize(statement,
select, resolver);
+ }
+ else {
+ select = JoinCompiler.optimize(statement, select,
this.resolver);
+ }
+ if (this.select != select) {
+ ColumnResolver resolver1 =
FromCompiler.getResolverForQuery(select, statement.getConnection());
+ context = new StatementContext(statement,
resolver1, scan, sequenceManager);
+ }
+ JoinTable joinTable = JoinCompiler.compile(statement,
select, context.getResolver());
+ return compileJoinQuery(context, binds, joinTable,
false, false, null);
+ } else {
+ return compileSingleQuery(context, select, binds, false,
true);
+ }
+}
+
+private void checkForOrderByLimitInUnionAllSelect(SelectStatement
select) throws SQLException {
+if (select.getOrderBy() != null !select.getOrderBy().isEmpty())
{
+throw new
SQLExceptionInfo.Builder(SQLExceptionCode.ORDER_BY_IN_UNIONALL_SELECT_NOT_SUPPORTED).setMessage(.).build().buildException();
+}
+if (select.getLimit() != null) {
+throw new
SQLExceptionInfo.Builder(SQLExceptionCode.LIMIT_IN_UNIONALL_SELECT_NOT_SUPPORTED).setMessage(.).build().buildException();
+}
+}
+
+private PTable createTempTableForUnionAllResultResolver(QueryPlan
plan) throws SQLException {
+ListPColumn projectedColumns = new ArrayListPColumn();
+Long scn = statement.getConnection().getSCN();
+ListPColumnFamily families =
Collections.PColumnFamilyemptyList(); // new ArrayListPColumnFamily();
+PTable theTable = new
PTableImpl(statement.getConnection().getTenantId(), unionAllSchema,
unionAllTable, scn == null ? HConstants.LATEST_TIMESTAMP : scn, families);
+PTable table = plan.getTableRef().getTable();
+for (int i=0; i plan.getProjector().getColumnCount(); i++) {
+ColumnProjector colProj =
plan.getProjector().getColumnProjector(i);
+Expression sourceExpression = colProj.getExpression();
+PColumnImpl projectedColumn = new
PColumnImpl(PNameFactory.newName(colProj.getName().getBytes()),
table.getDefaultFamilyName(),
+sourceExpression.getDataType(),
sourceExpression.getMaxLength(), sourceExpression.getScale(),
sourceExpression.isNullable(),
+i, sourceExpression.getSortOrder(), 50, new byte[0],
true, sourceExpression.toString());
+projectedColumns.add(projectedColumn);
+}
+PTable t = PTableImpl.makePTable(theTable, projectedColumns);
+return t;
+}
+
+private QueryPlan buildTupleProjectPlan(QueryPlan plan) throws
SQLException {
+ListExpressionProjector projectedColumns = new
ArrayListExpressionProjector();
+PTable tbl = createTempTableForUnionAllResultResolver(plan);
+for (int i=0; itbl.getColumns().size(); i++) {
+ProjectedColumnExpression expression = new
ProjectedColumnExpression(tbl.getColumns().get(i), tbl.getColumns(), i,
tbl.getColumns().get(i).getExpressionStr
[GitHub] phoenix pull request: Phoenix-1580 union all impl
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/39#discussion_r25924080
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -147,21 +169,146 @@ public QueryCompiler(PhoenixStatement statement,
SelectStatement select, ColumnR
*/
public QueryPlan compile() throws SQLException{
SelectStatement select = this.select;
-ListObject binds = statement.getParameters();
+QueryPlan plan;
+if (isUnionAll()) {
+plan = compileUnionAll(select);
+} else {
+plan = compileSelect(select);
+}
+return plan;
+}
+
+public QueryPlan compileSelect(SelectStatement select) throws
SQLException{
+ // SelectStatement select = this.select;
+ ListObject binds = statement.getParameters();
+ StatementContext context = new StatementContext(statement,
resolver, scan, sequenceManager);
+ ColumnResolver resolver;
+ if (isUnionAll()) {
+ resolver = FromCompiler.getResolverForQuery(select,
statement.getConnection());
+ context = new StatementContext(statement, resolver, new
Scan(), sequenceManager);
+ }
+ if (select.isJoin()) {
+ if (isUnionAll()) {
+ resolver =
FromCompiler.getResolverForQuery(select, statement.getConnection());
+ select = JoinCompiler.optimize(statement,
select, resolver);
+ }
+ else {
+ select = JoinCompiler.optimize(statement, select,
this.resolver);
+ }
+ if (this.select != select) {
+ ColumnResolver resolver1 =
FromCompiler.getResolverForQuery(select, statement.getConnection());
+ context = new StatementContext(statement,
resolver1, scan, sequenceManager);
+ }
+ JoinTable joinTable = JoinCompiler.compile(statement,
select, context.getResolver());
+ return compileJoinQuery(context, binds, joinTable,
false, false, null);
+ } else {
+ return compileSingleQuery(context, select, binds, false,
true);
+ }
+}
+
+private void checkForOrderByLimitInUnionAllSelect(SelectStatement
select) throws SQLException {
+if (select.getOrderBy() != null !select.getOrderBy().isEmpty())
{
+throw new
SQLExceptionInfo.Builder(SQLExceptionCode.ORDER_BY_IN_UNIONALL_SELECT_NOT_SUPPORTED).setMessage(.).build().buildException();
+}
+if (select.getLimit() != null) {
+throw new
SQLExceptionInfo.Builder(SQLExceptionCode.LIMIT_IN_UNIONALL_SELECT_NOT_SUPPORTED).setMessage(.).build().buildException();
+}
+}
+
+private PTable createTempTableForUnionAllResultResolver(QueryPlan
plan) throws SQLException {
+ListPColumn projectedColumns = new ArrayListPColumn();
+Long scn = statement.getConnection().getSCN();
+ListPColumnFamily families =
Collections.PColumnFamilyemptyList(); // new ArrayListPColumnFamily();
+PTable theTable = new
PTableImpl(statement.getConnection().getTenantId(), unionAllSchema,
unionAllTable, scn == null ? HConstants.LATEST_TIMESTAMP : scn, families);
+PTable table = plan.getTableRef().getTable();
+for (int i=0; i plan.getProjector().getColumnCount(); i++) {
+ColumnProjector colProj =
plan.getProjector().getColumnProjector(i);
+Expression sourceExpression = colProj.getExpression();
+PColumnImpl projectedColumn = new
PColumnImpl(PNameFactory.newName(colProj.getName().getBytes()),
table.getDefaultFamilyName(),
+sourceExpression.getDataType(),
sourceExpression.getMaxLength(), sourceExpression.getScale(),
sourceExpression.isNullable(),
+i, sourceExpression.getSortOrder(), 50, new byte[0],
true, sourceExpression.toString());
+projectedColumns.add(projectedColumn);
+}
+PTable t = PTableImpl.makePTable(theTable, projectedColumns);
+return t;
+}
+
+private QueryPlan buildTupleProjectPlan(QueryPlan plan) throws
SQLException {
+ListExpressionProjector projectedColumns = new
ArrayListExpressionProjector();
+PTable tbl = createTempTableForUnionAllResultResolver(plan);
+for (int i=0; itbl.getColumns().size(); i++) {
+ProjectedColumnExpression expression = new
ProjectedColumnExpression(tbl.getColumns().get(i), tbl.getColumns(), i,
tbl.getColumns().get(i).getExpressionStr
[GitHub] phoenix pull request: Phoenix-1580 union all impl
Github user maryannxue commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/39#discussion_r25924146
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java ---
@@ -147,21 +169,146 @@ public QueryCompiler(PhoenixStatement statement,
SelectStatement select, ColumnR
*/
public QueryPlan compile() throws SQLException{
SelectStatement select = this.select;
-ListObject binds = statement.getParameters();
+QueryPlan plan;
+if (isUnionAll()) {
+plan = compileUnionAll(select);
+} else {
+plan = compileSelect(select);
+}
+return plan;
+}
+
+public QueryPlan compileSelect(SelectStatement select) throws
SQLException{
+ // SelectStatement select = this.select;
+ ListObject binds = statement.getParameters();
+ StatementContext context = new StatementContext(statement,
resolver, scan, sequenceManager);
+ ColumnResolver resolver;
+ if (isUnionAll()) {
+ resolver = FromCompiler.getResolverForQuery(select,
statement.getConnection());
+ context = new StatementContext(statement, resolver, new
Scan(), sequenceManager);
+ }
+ if (select.isJoin()) {
+ if (isUnionAll()) {
+ resolver =
FromCompiler.getResolverForQuery(select, statement.getConnection());
+ select = JoinCompiler.optimize(statement,
select, resolver);
+ }
+ else {
+ select = JoinCompiler.optimize(statement, select,
this.resolver);
+ }
+ if (this.select != select) {
+ ColumnResolver resolver1 =
FromCompiler.getResolverForQuery(select, statement.getConnection());
+ context = new StatementContext(statement,
resolver1, scan, sequenceManager);
+ }
+ JoinTable joinTable = JoinCompiler.compile(statement,
select, context.getResolver());
+ return compileJoinQuery(context, binds, joinTable,
false, false, null);
+ } else {
+ return compileSingleQuery(context, select, binds, false,
true);
+ }
+}
+
+private void checkForOrderByLimitInUnionAllSelect(SelectStatement
select) throws SQLException {
+if (select.getOrderBy() != null !select.getOrderBy().isEmpty())
{
+throw new
SQLExceptionInfo.Builder(SQLExceptionCode.ORDER_BY_IN_UNIONALL_SELECT_NOT_SUPPORTED).setMessage(.).build().buildException();
+}
+if (select.getLimit() != null) {
+throw new
SQLExceptionInfo.Builder(SQLExceptionCode.LIMIT_IN_UNIONALL_SELECT_NOT_SUPPORTED).setMessage(.).build().buildException();
+}
+}
+
+private PTable createTempTableForUnionAllResultResolver(QueryPlan
plan) throws SQLException {
--- End diff --
The implementation is pretty good, but should be called elsewhere. Please
refer to the comment below.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: PHOENIX-852 Optimize child/parent foreign ke...
GitHub user maryannxue opened a pull request: https://github.com/apache/phoenix/pull/7 PHOENIX-852 Optimize child/parent foreign key joins You can merge this pull request into a Git repository by running: $ git pull https://github.com/maryannxue/phoenix master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/7.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 #7 commit c89d815f64bba2cd85a2b03ed200338f9a47f63e Author: maryannxue maryann...@apache.org Date: 2014-08-19T03:04:00Z PHOENIX-852 Optimize child/parent foreign key joins --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
