forcedotcom / phoenix

BSD 3-Clause "New" or "Revised" License
558 stars 227 forks source link

CASE expression returning ARRAY doesn't have the correct type #653

Closed jtaylor-sfdc closed 10 years ago

jtaylor-sfdc commented 10 years ago

The following test in TestArray fails, but should succeed as the type returned by the CASE expression should be an ARRAY:

@Test
public void testCaseWithArray() throws Exception {
    long ts = nextTimestamp();
    String tenantId = getOrganizationId();
    createTableWithArray(BaseConnectedQueryTest.getUrl(),
            getDefaultSplits(tenantId), null, ts - 2);
    initTablesWithArrays(tenantId, null, ts);
    String query = "SELECT CASE WHEN A_INTEGER = 1 THEN a_double_array ELSE null END [2] FROM table_with_array";
    Properties props = new Properties(TEST_PROPERTIES);
    props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB,
            Long.toString(ts + 2)); // Execute at timestamp 2
    Connection conn = DriverManager.getConnection(PHOENIX_JDBC_URL, props);
    try {
        PreparedStatement statement = conn.prepareStatement(query);
        ResultSet rs = statement.executeQuery();
        assertTrue(rs.next());
        // Need to support primitive
        Double[] doubleArr = new Double[1];
        doubleArr[0] = 37.56;
        Double result =  rs.getDouble(1);
        assertEquals(result, doubleArr[0]);
        assertFalse(rs.next());
    } finally {
        conn.close();
    }
}
ramkrish86 commented 10 years ago

Am getting this when i try to execute this com.salesforce.phoenix.exception.PhoenixParserException: ERROR 603 (42P00): Syntax error. Mismatched input. Expecting "FROM", got "[" at line 1, column 66. at com.salesforce.phoenix.parse.SQLParser.parseStatement(SQLParser.java:116) at com.salesforce.phoenix.jdbc.PhoenixStatement$PhoenixStatementParser.parseStatement(PhoenixStatement.java:868) at com.salesforce.phoenix.jdbc.PhoenixStatement.parseStatement(PhoenixStatement.java:935) at com.salesforce.phoenix.jdbc.PhoenixPreparedStatement.(PhoenixPreparedStatement.java:85) at com.salesforce.phoenix.jdbc.PhoenixConnection.prepareStatement(PhoenixConnection.java:443) at com.salesforce.phoenix.end2end.ArrayTest.testCaseWithArray(ArrayTest.java:249) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner.run(ParentRunner.java:309) at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197) Caused by: MismatchedTokenException(73!=50) at com.salesforce.phoenix.parse.PhoenixSQLParser.recoverFromMismatchedToken(PhoenixSQLParser.java:306) at org.antlr.runtime.BaseRecognizer.match(BaseRecognizer.java:115) at com.salesforce.phoenix.parse.PhoenixSQLParser.select_node(PhoenixSQLParser.java:2917) at com.salesforce.phoenix.parse.PhoenixSQLParser.hinted_select_node(PhoenixSQLParser.java:3076) at com.salesforce.phoenix.parse.PhoenixSQLParser.oneStatement(PhoenixSQLParser.java:542) at com.salesforce.phoenix.parse.PhoenixSQLParser.statement(PhoenixSQLParser.java:448) at com.salesforce.phoenix.parse.SQLParser.parseStatement(SQLParser.java:113) ... 31 more

But when the query is "SELECT CASE WHEN A_INTEGER = 1 THEN a_double_array[2] ELSE null END FROM table_with_array" it works fine. The testcase passes. Did u intend to add [2] after END and before FROM?

jtaylor-sfdc commented 10 years ago

Try again, as my grammar change is checked in now (as is the above test, but with an @Ignore). The grammar change allows an array to be returned by an expression (i.e. remember how we were trying to allow array references to be expressions in the grammar?).

So the idea is that you can have an expression, like a CASE statement or a built-in function return an ARRAY type, like this:

SELECT CASE WHEN a=0 THEN a_array WHEN a=1 THEN b_array ELSE c_array END [the_index] FROM t

In this example, the CASE expression should evaluate to an array. It could have been written like this too:

SELECT ARRAY_ELEM(CASE WHEN a=0 THEN a_array WHEN a=1 THEN b_array ELSE c_array END, the_index) FROM t

But I'm getting a compile time error. Take a look at the following method in CaseExpression:

private static List<Expression> coerceIfNecessary(List<Expression> children) throws SQLException {
    boolean isChildTypeUnknown = false;
    PDataType returnType = children.get(0).getDataType();
    ...

The getDataType() should return an ARRAY PDataType, but my guess is that it's returning the element type instead.

ramkrish86 commented 10 years ago

Ok let me check this now.

ramkrish86 commented 10 years ago

This is what is happening, {code} boolean isChildTypeUnknown = false; PDataType returnType = children.get(0).getDataType(); {code} Here it is coming as array only. Since the ELSE condition says to make it null {code} if (isChildTypeUnknown && returnType.isCoercibleTo(PDataType.DECIMAL)) { returnType = PDataType.DECIMAL; } {code} And this is converting the expression to TO_DECIMAL(A_DOUBLE_ARRAY). The index position does not come into effect here. Now since TO_DECIMAL becomes an inbuilt function it now calls for isCoercible method where DECIMAL is checked with ARRAY which happens in FunctionParseNode.validate() method. This is where error is thrown. Checking out where could be the problem.

ramkrish86 commented 10 years ago

In syntaxes like this {code} SELECT CASE WHEN a=0 THEN a_array WHEN a=1 THEN b_array ELSE c_array END [the_index] FROM t {code} where b_array is double_array and c_array is string_array then {code} throw new SQLExceptionInfo.Builder(SQLExceptionCode.CANNOT_CONVERT_TYPE) .setMessage("Case expressions must have common type: " + returnType + " cannot be coerced to " + childType) .build().buildException(); {code} I think this is valid, right?

ramkrish86 commented 10 years ago

I made this change {code} if (isChildTypeUnknown && returnType.isCoercibleTo(PDataType.DECIMAL)) { if (!returnType.isArrayType()) { returnType = PDataType.DECIMAL; } } {code} In CaseExpression.coerceIfNecessary, then it passes. Do let me know if this change is fine. I feel it is ok because with arrays we cannot convert it into TO_DECIMAL function. What do you think? I may be wrong here. Pls do correct me.

jtaylor-sfdc commented 10 years ago

The check for returnType.isCoercibleTo(PDataType.DECIMAL) should fail when returnType is an array type. An array type should never be coercible to a primitive type. The isCoercibleTo check is asking "can I cast type1 to type2".

ramkrish86 commented 10 years ago

Hmm.. my bad.. I have given a fix in my new pull request.

jtaylor-sfdc commented 10 years ago

Fixed now by @ramkrish86. Thanks for the contributions!