Mailing-List: contact dev-help@phoenix.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@phoenix.apache.org
Date: Fri, 3 Apr 2015 17:57:54 +0000 (UTC)
From: "James Taylor (JIRA)" <jira@apache.org>
To: dev@phoenix.incubator.apache.org
Message-ID: <JIRA.12767246.1421179567000.119566.1428083874567@Atlassian.JIRA>
In-Reply-To: <JIRA.12767246.1421179567000@Atlassian.JIRA>
References: <JIRA.12767246.1421179567000@Atlassian.JIRA>
 <JIRA.12767246.1421179567568@arcas>
Subject: [jira] [Commented] (PHOENIX-1580) Support UNION ALL
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/PHOENIX-1580?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14394775#comment-14394775 ] 

James Taylor commented on PHOENIX-1580:
---------------------------------------

bq. Need to check null otherwise tests could fail
Which tests fail without the null check? If none, please remove.

Please makes sure that all of [~maryannxue]'s changes are part of your patch. See her #1 and #3 above.

[~ayingshu] - why do we keep having to repeat ourselves? Again and again, I have to review the same code. I'm only reviewing this patch once more today, and only once per day after that. There have been over 100 comments to attempt to get this complete.

This test makes no sense and will fail one way or another:
{code}
+    @Test
+    public void testUnionAllInSubquery() throws Exception {
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        Connection conn = DriverManager.getConnection(getUrl(), props);
+        conn.setAutoCommit(false);
+
+        try {
+            String ddl = "CREATE TABLE test_table " +
+                    "  (a_string varchar not null, col1 integer" +
+                    "  CONSTRAINT pk PRIMARY KEY (a_string))\n";
+            createTestTable(getUrl(), ddl);
+
+            ddl = "CREATE TABLE b_table " +
+                    "  (a_string varchar not null, col1 integer" +
+                    "  CONSTRAINT pk PRIMARY KEY (a_string))\n";
+            createTestTable(getUrl(), ddl);
+
+            ddl = "select a_string, col1 from test_table where a_string in (select a_string from test_table union all select a_string from b_table)";
+            ResultSet rs = conn.createStatement().executeQuery(ddl);
+            assertTrue(rs.next());
+            fail();
+        } finally {
+            conn.close();
+        }
+    }
+
{code}

You need to add a catch SQLFeatureNotSupportedException. You shouldn't need the assertTrue(rs.next()) either, as the SQLFeatureNotSupportedException should happen when the query is compiled during the executeQuery call.

The explain plan doesn't look correct:
{code}
+            assertEquals("UNION ALL 2 queries\n" + "\n" + 
+                    "CLIENT PARALLEL 1-WAY FULL SCAN OVER TEST_TABLE\n" +
+                    "    SERVER TOP 1 ROW SORTED BY [COL1]\n" +
+                    "CLIENT MERGE SORT\n" +
+                    "CLIENT PARALLEL 1-WAY FULL SCAN OVER B_TABLE\n" +
+                    "    SERVER TOP 1 ROW SORTED BY [COL1]\n" +
+                    "CLIENT MERGE SORT\n" +
+                    "    SERVER TOP 1 ROW SORTED BY [COL1]\n" +
+                    "CLIENT MERGE SORT", QueryUtil.getExplainPlan(rs)); 
{code}
- One minor nit: change "UNION ALL 2 queries" to "UNION ALL OVER 2 QUERIES".
- There's an extra SERVER TOP 1 ROW SORTED BY [COL1] in the plan. Easiest fix would be to pass in a boolean clientSideOnly flag to MergeSortTopNResultIterator that causes that line not to be output. An alternative would be to create a subclass of MergeSortTopNResultIterator called ClientMergeSortTopNResultIterator that overides that method.

Have you run all unit tests with "mvn verify"? Based on the above test, I think the answer is probably no, so please run that before submitting an updated patch.

> Support UNION ALL
> -----------------
>
>                 Key: PHOENIX-1580
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-1580
>             Project: Phoenix
>          Issue Type: Improvement
>            Reporter: Alicia Ying Shu
>            Assignee: Alicia Ying Shu
>         Attachments: PHOENIX-1580-grammar.patch, Phoenix-1580-v1.patch, Phoenix-1580-v2.patch, Phoenix-1580-v3.patch, Phoenix-1580-v4.patch, Phoenix-1580-v5.patch, Phoenix-1580-v6.patch, Phoenix-1580-v7.patch, Phoenix-1580-v8.patch, phoenix-1580-v1-wipe.patch, phoenix-1580.patch, unionall-wipe.patch
>
>
> Select * from T1
> UNION ALL
> Select * from T2


--
This message was sent by Atlassian JIRA
(v6.3.4#6332)