You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by d9liang <gi...@git.apache.org> on 2015/09/29 00:44:01 UTC
[GitHub] phoenix pull request: Phoenix 572 Truncate CHAR with max length on...
GitHub user d9liang opened a pull request:
https://github.com/apache/phoenix/pull/117
Phoenix 572 Truncate CHAR with max length on upsert
This transaction truncate string to the CHAR max length value on upsert. A test case is added and passed. Please review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/d9liang/phoenix PHOENIX-572
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/117.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 #117
----
commit 58d261058a23f08f4a906f049aa164f8def4e807
Author: Dongming Liang <do...@capitalone.com>
Date: 2015-04-07T19:39:04Z
revise test
commit 255c68075e4e58a5801eca32d455ac30d2ec8b18
Author: Dongming Liang <do...@capitalone.com>
Date: 2015-09-23T19:52:31Z
add RoundFloorCeilExpressionTest.java
commit 8bf9e629053d32b6c6e0f920f385f036815c9720
Author: Dongming Liang <do...@capitalone.com>
Date: 2015-09-28T22:08:08Z
merge upstream and clean up
commit 685097f2714694b7ba2667638a5e8b6e4e87f4f1
Author: Dongming Liang <do...@capitalone.com>
Date: 2015-09-28T22:27:12Z
remove non-relevant test in RoundFloorCeilExpressionsTest.java
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 572 Truncate CHAR with max length on...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the pull request:
https://github.com/apache/phoenix/pull/117#issuecomment-145189319
Thanks for the patch, @d9liang - I reviewed and left you some minor feedback. Overall, it looks very good. One other thing to check (and add tests for) is other code paths that insert data: namely UPSERT SELECT (on client-side and server-side) and CSV bulk load (which goes through a similar code path as UPSERT VALUES, so you might already be covered there, not sure). This would ensure that we're consistently truncating fixed width types in all cases.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 572 Truncate CHAR with max length on...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/117#discussion_r41081112
--- Diff: phoenix-core/src/test/java/org/apache/phoenix/query/UpsertQueryTest.java ---
@@ -0,0 +1,92 @@
+/*
+ * 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.query;
+
+import com.google.common.collect.Maps;
+import org.apache.phoenix.end2end.BaseHBaseManagedTimeIT;
+import org.apache.phoenix.end2end.Shadower;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.query.BaseConnectionlessQueryTest;
+import org.apache.phoenix.query.BaseTest;
+
+import java.sql.*;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Tests for Upsert Statement Compilation
+ * testUpsertTruncChar() String should be truncated to max CHAR size
+ */
+
+public class UpsertQueryTest extends BaseTest {
+
+ @Test
+ public void testUpsertTruncChar() throws Exception {
+ Connection conn = DriverManager.getConnection(getUrl());
+
+ String testTable = "phoenix572";
+ String origStr = "abcde";
+ String expectedStr = "abc";
+ int charSize = 3;
+ String stmt;
+
+ stmt = "CREATE TABLE " + testTable + "(\n" +
+ "k1 VARCHAR NOT NULL PRIMARY KEY, v1 CHAR(" +
+ charSize + "))";
+ conn.createStatement().execute(stmt);
+
+ conn.createStatement().executeUpdate("UPSERT INTO " + testTable +
+ " values('x', '" + origStr + "')");
+ conn.commit();
+
+ stmt = "SELECT v1, length(v1) FROM " + testTable;
+ ResultSet rs = conn.createStatement().executeQuery(stmt);
+
+ assertTrue(rs.next());
+ assertEquals(expectedStr, rs.getString(1));
+ assertEquals(charSize, rs.getInt(2));
+
+ stmt = "DROP TABLE " + testTable;
+ conn.createStatement().execute(stmt);
+ conn.close();
+ }
+
+ @BeforeClass
--- End diff --
Both the doSetup() and doTeardown() overrides can be removed once you derived from BaseHBaseTimeManagedIT.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 572 Truncate CHAR with max length on...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/117#discussion_r41081079
--- Diff: phoenix-core/src/test/java/org/apache/phoenix/query/UpsertQueryTest.java ---
@@ -0,0 +1,92 @@
+/*
+ * 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.query;
+
+import com.google.common.collect.Maps;
+import org.apache.phoenix.end2end.BaseHBaseManagedTimeIT;
+import org.apache.phoenix.end2end.Shadower;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.query.BaseConnectionlessQueryTest;
+import org.apache.phoenix.query.BaseTest;
+
+import java.sql.*;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/**
+ * Tests for Upsert Statement Compilation
+ * testUpsertTruncChar() String should be truncated to max CHAR size
+ */
+
+public class UpsertQueryTest extends BaseTest {
--- End diff --
Extend from BaseHBaseTimeManagedIT instead of BaseTest directly.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request: Phoenix 572 Truncate CHAR with max length on...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/117#discussion_r41081051
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java ---
@@ -787,15 +789,26 @@ public MutationState execute() throws SQLException {
constantExpression.getDataType(), column.getDataType(), "expression: "
+ constantExpression.toString() + " in column " + column);
}
- if (!column.getDataType().isSizeCompatible(ptr, value, constantExpression.getDataType(),
+ if ((column.getDataType() != PChar.INSTANCE) && // Char is truncated later
+ !column.getDataType().isSizeCompatible(ptr, value, constantExpression.getDataType(),
--- End diff --
Is there a reason we wouldn't want to truncate the value for *all* fixed width types (i.e. column.getDataType().isFixedWidth)? In general, we try not to match on a particular type, but rather on categories of types. That way, when a new type gets added, it behaves a bit better.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---