PHOENIX-4981 Add tests for ORDER BY, GROUP BY and salted tables using… #402

twdsilva · 2018-10-30T20:59:26Z

… phoenix-spark]

@karanmehta93 thanks for the review nice catch, I modified the SparkContext variable to be volatile.
@ChinmaySKulkarni can you please review? I refactored the AggregateIT, OrderByIT and SaltedIT so that it can be used to run queries using phoenix-spark. I created Base*IT based on these tests with two subclasses (one for phoenix , one for phoenix-spark). I added a QueryBuilder to generate a SQL query that is used to setup the spark sql query. I also added SparkResultSet that implements the JDBC interface so that the existing tests can be reused without much changes.

twdsilva · 2018-10-30T23:30:46Z

I also had to bump up the spark version to 2.3.2 as this version has more sql support, in order to get the tests to pass.

ChinmaySKulkarni · 2018-10-31T03:31:21Z

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

+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;


nit: Looks like this diff is just due to reordering imports. Please refactor

ChinmaySKulkarni · 2018-10-31T04:21:14Z

phoenix-core/src/main/java/org/apache/phoenix/util/QueryBuilder.java

+    private HintNode.Hint hint;
+    private boolean escapeCols;
+    private boolean distinct;
+    private int limit;


Can we make all member variables final and have the builder's build method set all values in a private constructor so that we follow the builder pattern more closely?

IMHO I don't think that necessary as we don't have an object that represents a Query, the build() just returns a a string.

Ok makes sense

ChinmaySKulkarni · 2018-10-31T04:26:07Z

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

-        List<KeyRange> splits = TestUtil.getAllSplits(conn, tableName);
-        assertEquals(nGuidePosts, splits.size());
-    }
-
    @Test
    public void testGroupByWithAliasWithSameColumnName() throws SQLException {


Why is this test case not applicable to phoenix-spark?

The query test joins. QueryBuilder currently doesn't have support to generate a join query over two tables.

ChinmaySKulkarni · 2018-10-31T04:32:15Z

phoenix-spark/pom.xml

@@ -487,6 +487,16 @@
    <testSourceDirectory>src/it/scala</testSourceDirectory>
    <testResources><testResource><directory>src/it/resources</directory></testResource></testResources>
    <plugins>
+<!--


Remove this commented section

I had commented this by mistake, fixed now.

ChinmaySKulkarni · 2018-10-31T04:34:32Z

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

+        catch(Exception e) {
+            assertTrue(e.getMessage().contains(expectedPhoenixExceptionMsg));
+        }
+        return rs;


Do we ever want code to reach here? Or do we want to Assert.fail if the exception doesn't occur?

we should fail if an exception is not thrown, I fixed this.

ChinmaySKulkarni · 2018-10-31T04:38:24Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/AggregateIT.java

+            sql="select count(*) from "+intTableName;
+            QueryBuilder queryBuilder = new QueryBuilder();
+            queryBuilder.setSelectExpression("COUNT(*)");
+            queryBuilder.setFullTableName(intTableName);


You can instead do method chaining here since you have a fluent interface.

ChinmaySKulkarni · 2018-10-31T04:49:52Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/AggregateIT.java

+            assertEquals(1, rs.getLong(1));
+
+            sql="select count(*) from "+intTableName + " where b.colb is null";
+            queryBuilder.setWhereClause("`B.COLB` IS NULL");


It seems like the only difference between this test and the one in phoenix-core is the backticks provided to querybuilder setter methods. I'm guessing this is a result of how SparkUtil executes queries. Please correct me if I'm wrong, but if not, can we further reuse the code from the phoenix-core tests instead of having more duplication?

If a column name contains a dot that spark sql requires the backticks. Automatically generating the sql for this is difficult especially when columns as part of expressions etc.

Ok let's leave it the way it is for now.

ChinmaySKulkarni · 2018-10-31T04:50:40Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/AggregateIT.java

+            String expectedPhoenixExceptionMsg, String expectedSparkExceptionMsg) {
+        ResultSet rs = null;
+        try {
+            rs = executeQuery(conn, queryBuilder);


Similar question/comment about Assert.fail here as well.

ChinmaySKulkarni · 2018-10-31T04:57:24Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/OrderByIT.java

+            Dataset phoenixDataSet =
+                    new PhoenixRDD(SparkUtil.getSparkContext(), tableName1,
+                            JavaConverters.collectionAsScalaIterableConverter(table1Columns)
+                                    .asScala().toSeq(),


Can you add some comments here

ChinmaySKulkarni · 2018-10-31T05:05:34Z

phoenix-spark/src/main/java/org/apache/phoenix/spark/SparkResultSet.java

+import java.util.Map;
+
+/**
+ * Helper class to convert a List of Rows returns from a dataset to a sql ResultSet


nit: 'returned'

ChinmaySKulkarni · 2018-10-31T05:12:56Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/SparkUtil.java

+
+    public static ResultSet executeQuery(Connection conn, QueryBuilder queryBuilder, String url, Configuration config)
+            throws SQLException {
+        SQLContext sqlContext = new SQLContext(SparkUtil.getSparkContext());


It looks as though SQLContext is deprecated. Quoting: Deprecated. Use SparkSession.builder instead. Since 2.0.0.

ChinmaySKulkarni · 2018-10-31T05:37:30Z

@twdsilva some comments and questions. Overall looks really good!

twdsilva · 2018-11-01T00:35:35Z

@ChinmaySKulkarni Thanks for the review, I have updated the PR please take a look.

ChinmaySKulkarni · 2018-11-01T03:18:39Z

@twdsilva changes look good. Do we want to continue using SQLContext? Documentation says that it is deprecated and that SparkSession.builder should be used instead (since spark 2.0.0).

twdsilva · 2018-11-01T18:50:22Z

@ChinmaySKulkarni I removed the use of the deprecated SqlContext constructor, please take a look.

ChinmaySKulkarni · 2018-11-01T19:07:25Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/OrderByIT.java

+            stmt.execute();
+            conn.commit();
+
+            // create two PhoenixRDDs  using the table namea and columns that are required for the JOIN query


ChinmaySKulkarni · 2018-11-01T19:08:22Z

phoenix-spark/src/it/java/org/apache/phoenix/spark/SparkUtil.java

+    }
+
+    public static SQLContext getSqlContext() {
+        return SparkSession.builder().appName("Java Spark Tests").master("local[2]")


nit: Extract all these strings as static final member variables

ChinmaySKulkarni · 2018-11-01T19:09:02Z

@twdsilva A couple of minor nits, otherwise looks good to go. Thanks!

twdsilva · 2018-11-01T21:47:14Z

Thank @ChinmaySKulkarni , fixed the nits, will get this committed.

… phoenix-spark

ChinmaySKulkarni reviewed Oct 31, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4981 branch 2 times, most recently from 49cf9e8 to 565b157 Compare November 1, 2018 00:34

ChinmaySKulkarni reviewed Nov 1, 2018

View reviewed changes

twdsilva force-pushed the PHOENIX-4981 branch from 854a71a to 57ac77e Compare November 1, 2018 21:46

PHOENIX-4981 Add tests for ORDER BY, GROUP BY and salted tables using…

91fc5f9

… phoenix-spark

twdsilva force-pushed the PHOENIX-4981 branch from 57ac77e to 91fc5f9 Compare November 2, 2018 18:36

twdsilva closed this Nov 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-4981 Add tests for ORDER BY, GROUP BY and salted tables using… #402

PHOENIX-4981 Add tests for ORDER BY, GROUP BY and salted tables using… #402

twdsilva commented Oct 30, 2018

twdsilva commented Oct 30, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Nov 1, 2018

ChinmaySKulkarni Oct 31, 2018 •

edited

Loading

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Nov 1, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Nov 1, 2018

ChinmaySKulkarni Oct 31, 2018

twdsilva Oct 31, 2018

ChinmaySKulkarni Oct 31, 2018

ChinmaySKulkarni commented Oct 31, 2018

twdsilva commented Nov 1, 2018

ChinmaySKulkarni commented Nov 1, 2018

twdsilva commented Nov 1, 2018

ChinmaySKulkarni Nov 1, 2018

ChinmaySKulkarni Nov 1, 2018

ChinmaySKulkarni commented Nov 1, 2018

twdsilva commented Nov 1, 2018

PHOENIX-4981 Add tests for ORDER BY, GROUP BY and salted tables using… #402

PHOENIX-4981 Add tests for ORDER BY, GROUP BY and salted tables using… #402

Conversation

twdsilva commented Oct 30, 2018

twdsilva commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni Oct 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni commented Oct 31, 2018

twdsilva commented Nov 1, 2018

ChinmaySKulkarni commented Nov 1, 2018

twdsilva commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni commented Nov 1, 2018

twdsilva commented Nov 1, 2018

ChinmaySKulkarni Oct 31, 2018 •

edited

Loading