[pulsar-sql] Make partition as internal column #4888

codelipenghui · 2019-08-05T06:09:27Z

Motivation

Stop return partition name in table list, just return the partitioned topic name in table list. This will avoid huge tables while user create large number of partition.
Make partition as internal column, provide users with the ability to get which partition data in and filtration based on partition. For example:

SELECT * FROM "my-table" WHERE "__partition__" = 0;
SELECT * FROM "my-table" WHERE "__partition__" in (2,3);
SELECT * FROM "my-table" WHERE "__partition__" < 1;

Modifications

Add "partition" internal column.
Add domain handle for "partition".

Verifying this change

Added new unit test to verify this change

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (yes)
The schema: (no)
The default values of configurations: (no)
The wire protocol: ( no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (yes)

sijie · 2019-08-05T07:10:13Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarInternalColumn.java

+        }
+
+        @Override
+        public Object getData(RawMessage message) {


You can get partition from the messageId, no?

RawMessage does not contains partition now and get partition by split will reduce memory usage more than storing a partition per RawMessage.

sijie · 2019-08-05T07:12:59Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSplitManager.java

        }

-        int actualNumSplits = Math.max(numPartitions, numSplits);
+        List<Integer> predicatePartitions = new ArrayList<>();


Move line 152-181 to a function "getPredicatedPartitions" or "getFilteredPartitions"

codelipenghui · 2019-08-05T09:42:10Z

run cpp tests
run integration tests

sijie · 2019-08-05T10:34:28Z

@codelipenghui I think the tests are failing due to this pull request.


Test Name | Duration | Age
-- | -- | --
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPartitionedTopic | 55 ms | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPartitionedTopic | 36 ms | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPartitionedTopic | 46 ms | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPublishTimePredicatePushdownPartitionedTopic | 31 ms | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPublishTimePredicatePushdownPartitionedTopic | 30 ms | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPublishTimePredicatePushdownPartitionedTopic | 29 ms | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testTopic | 0.17 sec | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testTopic | 0.15 sec | 2
org.apache.pulsar.sql.presto.TestPulsarSplitManager.testTopic | 0.15 sec | 2

Error Message
java.lang.ArithmeticException: / by zero
Stacktrace
java.lang.RuntimeException: java.lang.ArithmeticException: / by zero
	at org.apache.pulsar.sql.presto.PulsarSplitManager.getSplits(PulsarSplitManager.java:132)
	at org.apache.pulsar.sql.presto.TestPulsarSplitManager.testPartitionedTopic(TestPulsarSplitManager.java:136)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.ArithmeticException: / by zero
	at org.apache.pulsar.sql.presto.PulsarSplitManager.getSplitsPartitionedTopic(PulsarSplitManager.java:148)
	at org.apache.pulsar.sql.presto.TestPulsarSplitManager$ResultCaptor.answer(TestPulsarSplitManager.java:69)
	at org.apache.pulsar.sql.presto.PulsarSplitManager.getSplits(PulsarSplitManager.java:127)
	... 9 more

jerrypeng · 2019-08-05T17:48:49Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSplitManager.java

@@ -388,4 +431,8 @@ public boolean apply(Entry entry) {
            }
        });
    }
+
+    public PulsarAdmin getPulsarAdmin() {


Do we need this?

jerrypeng · 2019-08-05T17:59:41Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSplitManager.java

+                            high = range.getHigh().getValueBlock().get().getInt(0, 0);
+                        }
+                        while (low <= high) {
+                            if (!predicatePartitions.contains(low)) {


Do we need this conditional check?

jerrypeng · 2019-08-05T18:01:33Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSplitManager.java

+                        if (!range.getHigh().isLowerUnbounded() && range.getHigh().getValueBlock().isPresent()) {
+                            high = range.getHigh().getValueBlock().get().getInt(0, 0);
+                        }
+                        while (low <= high) {


Simpler and easier to read if we just use a for loop e.g.

for (int i = low; i <= high; i++)

jerrypeng · 2019-08-05T18:23:45Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarRecordCursor.java

@@ -91,6 +92,7 @@
    // are empty or not
    private final long splitSize;
    private long entriesProcessed = 0;
+    private int partition = -1;


For non-partitioned topics, should the partition column be NULL? Currently, for internal columns, if there is not an applicable value, the value is displayed as "NULL"

Is it necessary to be consistent with the messageId? If not, agree use NULL for non-partitioned topics.

personally I am in favor of using -1 to be consistent with how we use it in MessageId and it make a consistent view for both partitioned topic and non-partitioned topic.

A partitioned topic has N partitions, a non-partitioned topic has one partition with partition id "-1".

I am ok with either. @merlimat any opinion?

after discussing with @karthikramasamy since he has experience with databases, it seems that "null" can have all sort of meanings. Perhaps its just better to use -1 for non-partitioned topics

jerrypeng

Generally looks good! I left a few comments

codelipenghui · 2019-08-05T23:19:53Z

@jerrypeng I have addressed your comments, please take a look.
@sijie I have fix the unit test issue, please take a look.

codelipenghui · 2019-08-06T07:28:41Z

run cpp tests
run integration tests
run java8 tests

jerrypeng · 2019-08-06T17:18:55Z

pulsar-sql/presto-pulsar/src/main/java/org/apache/pulsar/sql/presto/PulsarSplitManager.java

+                        }
+                        for (int i = low; i <= high; i++) {
+                            predicatePartitions.add(i);
+                            low++;


do we need to do this?

codelipenghui · 2019-08-07T14:28:49Z

run java8 tests
run cpp tests
run integration tests

jiazhai · 2019-08-28T06:27:13Z

conflict with #4890 mark as 2.4.1

Fixes #4785 ### Motivation 1. Stop return partition name in table list, just return the partitioned topic name in table list. This will avoid huge tables while user create large number of partition. 2. Make partition as internal column, provide users with the ability to get which partition data in and filtration based on partition. For example: ``` SELECT * FROM "my-table" WHERE "__partition__" = 0; SELECT * FROM "my-table" WHERE "__partition__" in (2,3); SELECT * FROM "my-table" WHERE "__partition__" < 1; ``` ### Modifications 1. Add "__partition__" internal column. 2. Add domain handle for "__partition__". ### Verifying this change Added new unit test to verify this change ### Does this pull request potentially affect one of the following parts: *If `yes` was chosen, please highlight the changes* - Dependencies (does it add or upgrade a dependency): (no) - The public API: (yes) - The schema: (no) - The default values of configurations: (no) - The wire protocol: ( no) - The rest endpoints: (no) - The admin cli options: (no) - Anything that affects deployment: (no) ### Documentation - Does this pull request introduce a new feature? (yes) (cherry picked from commit 5adc522)

codelipenghui requested review from merlimat, jerrypeng and sijie and removed request for merlimat August 5, 2019 06:09

codelipenghui self-assigned this Aug 5, 2019

codelipenghui added the area/sql label Aug 5, 2019

codelipenghui added this to the 2.5.0 milestone Aug 5, 2019

sijie requested changes Aug 5, 2019

View reviewed changes

codelipenghui added 2 commits August 5, 2019 15:31

Pulsar partition as internal column.

c920d15

Pulsar partition as internal column.

9e8fd54

codelipenghui force-pushed the sql_partition branch from 2b8d7b0 to 9e8fd54 Compare August 5, 2019 07:33

Add getSplitsNonPartitionedTopic()

f3a4613

fix unit tests.

8ded62c

jerrypeng reviewed Aug 5, 2019

View reviewed changes

jerrypeng requested changes Aug 5, 2019

View reviewed changes

fix comments.

d3131f2

jerrypeng reviewed Aug 6, 2019

View reviewed changes

fix comments.

5ad6e99

codelipenghui force-pushed the sql_partition branch from 53dce27 to 5ad6e99 Compare August 8, 2019 02:36

fix style check.

dcf05ad

jerrypeng approved these changes Aug 9, 2019

View reviewed changes

sijie approved these changes Aug 10, 2019

View reviewed changes

sijie merged commit 5adc522 into apache:master Aug 10, 2019

codelipenghui deleted the sql_partition branch August 10, 2019 01:53

jiazhai modified the milestones: 2.5.0, 2.4.1 Aug 28, 2019

[pulsar-sql] Make partition as internal column #4888

[pulsar-sql] Make partition as internal column #4888

Conversation

codelipenghui commented Aug 5, 2019

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Aug 5, 2019

Uh oh!

sijie commented Aug 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerrypeng left a comment

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Aug 5, 2019

Uh oh!

codelipenghui commented Aug 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Aug 7, 2019

Uh oh!

jiazhai commented Aug 28, 2019

Uh oh!