You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by JeffreyLyonsD2L <gi...@git.apache.org> on 2015/08/05 17:44:29 UTC
[GitHub] phoenix pull request: PHOENIX-1673 Allow TenantId to be of any int...
GitHub user JeffreyLyonsD2L opened a pull request:
https://github.com/apache/phoenix/pull/104
PHOENIX-1673 Allow TenantId to be of any integral data type
Hopefully everything here is in order, if there are any questions or requests don't hesitate to ask.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/JeffreyLyonsD2L/phoenix PHOENIX-1673
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/104.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 #104
----
commit 9046050f983b0a3b412f3bf26e54948c612ee71f
Author: Jeffrey Lyons <je...@d2l.com>
Date: 2015-07-09T14:07:40Z
PHOENIX-1673 Allow TenantId to be of any integral data type
----
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L closed the pull request at:
https://github.com/apache/phoenix/pull/104
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-129218019
I can probably provide a little more information about our use case once I meet back with my team on Monday, but I can definitely say that James' point about being able to map existing tables with the concept of a tenantId without having to convert all of the data would be a big plus for us.
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128456464
Thanks so much for the contribution, @JeffreyLyonsD2L. Would you mind helping to review and commit this, @elilevine? I think the main thing to check is that there aren't any places in the code that Jeffrey missed updating.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36558688
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java ---
@@ -204,8 +204,18 @@ public final ResultIterator iterator(final List<? extends SQLCloseable> dependen
} else {
ScanUtil.setTimeRange(scan, context.getScanTimeRange());
}
-
- ScanUtil.setTenantId(scan, connection.getTenantId() == null ? null : connection.getTenantId().getBytes());
+ byte[] tenantIdBytes;
--- End diff --
Can you explain why you need the extra if/else here?
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128459657
Thanks for the ping, @JamesRTaylor. Haven't seen this PR until now. I'll review and commit today or tomorrow if everything looks good.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36559705
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java ---
@@ -204,8 +204,18 @@ public final ResultIterator iterator(final List<? extends SQLCloseable> dependen
} else {
ScanUtil.setTimeRange(scan, context.getScanTimeRange());
}
-
- ScanUtil.setTenantId(scan, connection.getTenantId() == null ? null : connection.getTenantId().getBytes());
+ byte[] tenantIdBytes;
--- End diff --
This is for a case where you have a tenant-specific connection set up and are accessing a table that is not multi-tenant, but it has a different type than your tenant ids. Without this check, the tenantId fails to convert and would prevent us from reading the table even though it is unrestricted.
---
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-1673 Allow TenantId to be of any int...
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36558704
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/MultiTenantTableIT.java ---
@@ -0,0 +1,140 @@
+/*
+ * 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.end2end;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.*;
+import java.util.Properties;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.Lists;
+
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.schema.TableAlreadyExistsException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class MultiTenantTableIT extends BaseClientManagedTimeIT {
+
+ private Connection regularConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ return DriverManager.getConnection(url, props);
+ }
+
+ private Connection tenantConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ String tenantIdProperty = this.tenantId.replaceAll("\'", "");
+ props.setProperty(PhoenixRuntime.TENANT_ID_ATTRIB, tenantIdProperty);
+ return DriverManager.getConnection(url, props);
+ }
+
+ private final String ddl;
+ private final String dataType;
+ private final String tenantId;
+ private final String otherTenantId;
+ private final String table;
+
+ public MultiTenantTableIT( String dataType, String tenantId, String otherTenantId ) {
+ this.dataType = dataType;
+ this.tenantId = tenantId;
+ this.otherTenantId = otherTenantId;
+ String tbl = "foo" + dataType;
+ if(tbl.contains("(")){
+ tbl = tbl.substring(0, tbl.indexOf("("));
+ }
+ this.table = tbl;
+ this.ddl = "create table " + table + " (" + "tid "+ dataType + " NOT NULL," + "id INTEGER NOT NULL, \n"
+ + "val VARCHAR " + "CONSTRAINT pk PRIMARY KEY(tid, id)) \n"
+ + "MULTI_TENANT=true";
+ }
+
+ @Parameters
+ public static Collection<Object[]> data() {
+ List<Object[]> testCases = Lists.newArrayList();
+ testCases.add(new Object[] { "INTEGER", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "UNSIGNED_INT", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "BIGINT", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "UNSIGNED_LONG", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "TINYINT", "127", "126" });
+ testCases.add(new Object[] { "UNSIGNED_TINYINT", "85", "84" });
+ testCases.add(new Object[] { "SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "UNSIGNED_SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "UNSIGNED_FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "UNSIGNED_DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "DECIMAL", "3.402823466", "3.402823465" });
+ testCases.add(new Object[] { "VARCHAR", "\'NameOfTenant\'", "\'Nemesis\'" });
+ testCases.add(new Object[] { "CHAR(10)", "\'1234567890\'", "\'Nemesis\'" });
+
+ return testCases;
+ }
+
+ @Test
+ public void testMultiTenantTables() throws Exception {
+ Connection conn = regularConnection(getUrl());
+ conn.setAutoCommit(true);
+ conn.createStatement().execute(ddl);
+
+
+ try {
+ conn.createStatement().execute(ddl);
--- End diff --
Is there a reason why you are calling conn.createStatement().execute(ddl) twice? Also no need to catch TableAlreadyExistsException since the constructor is called once per test method. Also the @After method in BaseHBaseManagedTimeIT takes care of dropping the table after every test method.
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128863633
This feature would allow the tenant ID to be represented through more types than just a VARCHAR. Some users may have tenants identified by an int or a long instead of a string. The tenant ID would still be passed as the stringified value, but the underlying table could use an int or a long as the column type. As you mentioned, the system tables (SYSTEM.CATALOG and SYSTEM.SEQUENCE) would need to continue to use the string representation. Every type can be converted to a string, so this should be ok (pending testing).
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130012385
Thanks @elilevine, it has been a very interesting discussion for me to see :)
With regards to the tests you mentioned earlier, I've added them to the TenantIdTypeIT. Although in looking over the review, I saw I missed one of your previous comments regarding testing tenant-specific indexes and sequences, so I'll add some of those this afternoon.
I'm fairly new to committing to open source, so is there anything you need from me organizationally or git-wise once the final code changes are in (something like going through and resolving the conflicts, or rebasing)?
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-129690118
Thanks for the details, @JeffreyLyonsD2L. I have to say I'm still torn on whether or not this Jira makes much sense. What if I told you that your concern #2 is already addressed? Currently tenant id columns can be either VARCHAR or fixed-length CHAR. Does that make it easier for you to use multi-tenancy in Phoenix?
Regarding your points #1 and 3, you are forced to do the conversion once per Connection when you set the TenantId parameter either way. Not sure what kind of conversion you mean beyond that. Normally when you read/write via a tenant-specific connection you only set TenantId on the Connection and don't need to actually write it or read it. Are you working with this data over non-tenant-specific connections?
---
I went though your code again. Everything looks good. The only thing missing now is:
1. Tests that create tenant views with non-string tenant id columns and test data upsertion/retrieval.
2. Same as above but negative. Test what happens when conversion fails.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130432243
You're very welcome, I've found the process streamlined and would be encouraged to contribute again! I resolved the conflicts on this branch, and created another PR for the 4.x-HBase-0.98 branch (#108 for reference)
I'll be available fore help if there is anything else needed.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-129580603
Everything looks good in terms of code. Excellent work, @JeffreyLyonsD2L. The only thing to figure out now is whether or not we should add the feature at all. I agree that changing the schema of SYSTEM.* tables is off the table. So do we allow the tenant id column in data tables to be of different type than in metadata tables? It does not sit well with me and makes multi-tenancy seem inconsistent IMHO, as I mentioned above, since tenant id is always passed in as string to connections and is stored as such in system tables.
@JeffreyLyonsD2L, maybe you can shine some light on our use-case. You mentioned that not having to do data conversion is a plus. In what way? Are you starting with existing data and don't want to do data conversion?
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36590856
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java ---
@@ -204,8 +204,18 @@ public final ResultIterator iterator(final List<? extends SQLCloseable> dependen
} else {
ScanUtil.setTimeRange(scan, context.getScanTimeRange());
}
-
- ScanUtil.setTenantId(scan, connection.getTenantId() == null ? null : connection.getTenantId().getBytes());
+ byte[] tenantIdBytes;
--- End diff --
In both cases we are getting it from the connection. In the if {} block we are using the table to figure out the RowKeySchema so we know what to convert it into.
The else might be unnecessary, I leaned towards keeping it in because then it mimicked the old behaviour in the case where you were scanning a regular table on a tenant-specific connection.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128830531
Good stuff, @JeffreyLyonsD2L. Added a few questions and comments to the PR.
There is one more issue that it not tackled here that we need to at least discuss IMHO. Phoenix's metadata tables, such as SYSTEM.CATALOG lead with TENANT_ID VARCHAR column (take a look in QueryConstants.java). This works well in a world where all multi-tenant tables can only have VARCHAR tenant id columns. Your changes introduce the possibility that different multi-tenant tables will have different types of tenant id columns.
What's the right solution here? The right solution is probably changing TENANT_ID column type to BINARY in system tables. But that will make upgrades for existing clusters a pain. Leave them as VARCHAR?
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36442703
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java ---
@@ -657,6 +660,22 @@ public static PName padTenantIdIfNecessary(RowKeySchema schema, boolean isSalted
return tenantId;
}
+ public static byte[] getTenantIdBytes(RowKeySchema schema, boolean isSalted, PName tenantId)
+ throws SQLException {
+ int pkPos = isSalted ? 1 : 0;
+ Field field = schema.getField(pkPos);
+ PDataType dataType = field.getDataType();
+ byte[] convertedValue;
+ try {
+ Object value = dataType.toObject(tenantId.getString());
--- End diff --
For this dataType.toBytes() call, use the one that accepts sortOrder so that it'll still work if the tenantId column is descending:
convertedValue = dataType.toBytes(value, field.getSortOrder());
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128840703
The TENANT_ID column in system tables is used to quickly filter metadata belonging to a tenant. Agree we could probably get away with leaving it VARCHAR because that is how tenant ids are passed to connections anyway.
Now that got me thinking... if we extending that logic further, why then do we need to implement support for other data types for tenant id columns if tenant ids are always passed in as strings to connections? Why not keep them all as VARCHAR/CHAR, like they are now?
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130002784
Makes sense, @JeffreyLyonsD2L. The existing data and integrations with other components interacting with the HBase tables do make the feature worth it IMHO. I'll commit after you add those tests. Thanks!
---
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-1673 Allow TenantId to be of any int...
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36558264
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/MultiTenantTableIT.java ---
@@ -0,0 +1,140 @@
+/*
+ * 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.end2end;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.*;
+import java.util.Properties;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.Lists;
+
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.schema.TableAlreadyExistsException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class MultiTenantTableIT extends BaseClientManagedTimeIT {
--- End diff --
Tests should extend BaseClientManagedTimeIT when they want to manage timestamps themselves, for ex- tests dealing with snapshot schema/queries. Your test class doesn't seem to be doing that. So make sure to extend BaseHBaseManagedTimeIT instead.
---
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-1673 Allow TenantId to be of any int...
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36558412
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/MultiTenantTableIT.java ---
@@ -0,0 +1,140 @@
+/*
+ * 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.end2end;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.*;
+import java.util.Properties;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.Lists;
+
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.schema.TableAlreadyExistsException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class MultiTenantTableIT extends BaseClientManagedTimeIT {
+
+ private Connection regularConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ return DriverManager.getConnection(url, props);
+ }
+
+ private Connection tenantConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ String tenantIdProperty = this.tenantId.replaceAll("\'", "");
+ props.setProperty(PhoenixRuntime.TENANT_ID_ATTRIB, tenantIdProperty);
+ return DriverManager.getConnection(url, props);
+ }
+
+ private final String ddl;
+ private final String dataType;
+ private final String tenantId;
+ private final String otherTenantId;
+ private final String table;
+
+ public MultiTenantTableIT( String dataType, String tenantId, String otherTenantId ) {
+ this.dataType = dataType;
+ this.tenantId = tenantId;
+ this.otherTenantId = otherTenantId;
+ String tbl = "foo" + dataType;
+ if(tbl.contains("(")){
+ tbl = tbl.substring(0, tbl.indexOf("("));
+ }
+ this.table = tbl;
+ this.ddl = "create table " + table + " (" + "tid "+ dataType + " NOT NULL," + "id INTEGER NOT NULL, \n"
+ + "val VARCHAR " + "CONSTRAINT pk PRIMARY KEY(tid, id)) \n"
+ + "MULTI_TENANT=true";
+ }
+
+ @Parameters
+ public static Collection<Object[]> data() {
+ List<Object[]> testCases = Lists.newArrayList();
+ testCases.add(new Object[] { "INTEGER", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "UNSIGNED_INT", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "BIGINT", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "UNSIGNED_LONG", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "TINYINT", "127", "126" });
+ testCases.add(new Object[] { "UNSIGNED_TINYINT", "85", "84" });
+ testCases.add(new Object[] { "SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "UNSIGNED_SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "UNSIGNED_FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "UNSIGNED_DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "DECIMAL", "3.402823466", "3.402823465" });
+ testCases.add(new Object[] { "VARCHAR", "\'NameOfTenant\'", "\'Nemesis\'" });
+ testCases.add(new Object[] { "CHAR(10)", "\'1234567890\'", "\'Nemesis\'" });
+
+ return testCases;
+ }
+
+ @Test
+ public void testMultiTenantTables() throws Exception {
+ Connection conn = regularConnection(getUrl());
--- End diff --
Include this in a try-with-resources construct to ensure connection is closed.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130400117
Good stuff, @JeffreyLyonsD2L! One last thing: can you update this pull request so it merges with Phoenix master cleanly (there are some conflicts now)? Also, would be great to see a PR for one of the 4.x branches if this patch does not merge cleanly there. You don't need to do it for each 4.x branch, one is fine. I'll take care of the rest. Again, thank you for your contributions.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130064362
Added the last few tests.
---
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-1673 Allow TenantId to be of any int...
Posted by samarthjain <gi...@git.apache.org>.
Github user samarthjain commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36558453
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/MultiTenantTableIT.java ---
@@ -0,0 +1,140 @@
+/*
+ * 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.end2end;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.*;
+import java.util.Properties;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.Lists;
+
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.schema.TableAlreadyExistsException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class MultiTenantTableIT extends BaseClientManagedTimeIT {
+
+ private Connection regularConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ return DriverManager.getConnection(url, props);
+ }
+
+ private Connection tenantConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ String tenantIdProperty = this.tenantId.replaceAll("\'", "");
+ props.setProperty(PhoenixRuntime.TENANT_ID_ATTRIB, tenantIdProperty);
+ return DriverManager.getConnection(url, props);
+ }
+
+ private final String ddl;
+ private final String dataType;
+ private final String tenantId;
+ private final String otherTenantId;
+ private final String table;
+
+ public MultiTenantTableIT( String dataType, String tenantId, String otherTenantId ) {
+ this.dataType = dataType;
+ this.tenantId = tenantId;
+ this.otherTenantId = otherTenantId;
+ String tbl = "foo" + dataType;
+ if(tbl.contains("(")){
+ tbl = tbl.substring(0, tbl.indexOf("("));
+ }
+ this.table = tbl;
+ this.ddl = "create table " + table + " (" + "tid "+ dataType + " NOT NULL," + "id INTEGER NOT NULL, \n"
+ + "val VARCHAR " + "CONSTRAINT pk PRIMARY KEY(tid, id)) \n"
+ + "MULTI_TENANT=true";
+ }
+
+ @Parameters
+ public static Collection<Object[]> data() {
+ List<Object[]> testCases = Lists.newArrayList();
+ testCases.add(new Object[] { "INTEGER", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "UNSIGNED_INT", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "BIGINT", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "UNSIGNED_LONG", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "TINYINT", "127", "126" });
+ testCases.add(new Object[] { "UNSIGNED_TINYINT", "85", "84" });
+ testCases.add(new Object[] { "SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "UNSIGNED_SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "UNSIGNED_FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "UNSIGNED_DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "DECIMAL", "3.402823466", "3.402823465" });
+ testCases.add(new Object[] { "VARCHAR", "\'NameOfTenant\'", "\'Nemesis\'" });
+ testCases.add(new Object[] { "CHAR(10)", "\'1234567890\'", "\'Nemesis\'" });
+
+ return testCases;
+ }
+
+ @Test
+ public void testMultiTenantTables() throws Exception {
+ Connection conn = regularConnection(getUrl());
+ conn.setAutoCommit(true);
+ conn.createStatement().execute(ddl);
+
+
+ try {
+ conn.createStatement().execute(ddl);
+ fail("Table with " + dataType + " tenantId not created correctly");
+ } catch (TableAlreadyExistsException e) {
+ // expected
+ }
+ conn.close();
+ conn = regularConnection(getUrl());
+ conn.setAutoCommit(true);
+
+ String query = "upsert into " + table +
+ " values (" + tenantId + ", 1 , 'valid')";
+
+ conn.createStatement().execute("upsert into " + table +
+ " values (" + tenantId + ", 1 , 'valid')");
+ conn.createStatement().execute("upsert into " + table +
+ " values (" + otherTenantId + ", 2 , 'invalid')");
+
+ Connection conn2 = tenantConnection(getUrl());
--- End diff --
Same as above - try-with-resources
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36442491
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/ScanUtil.java ---
@@ -657,6 +660,22 @@ public static PName padTenantIdIfNecessary(RowKeySchema schema, boolean isSalted
return tenantId;
}
+ public static byte[] getTenantIdBytes(RowKeySchema schema, boolean isSalted, PName tenantId)
+ throws SQLException {
+ int pkPos = isSalted ? 1 : 0;
+ Field field = schema.getField(pkPos);
+ PDataType dataType = field.getDataType();
+ byte[] convertedValue;
+ try {
+ Object value = dataType.toObject(tenantId.getString());
+ convertedValue = dataType.toBytes(value);
--- End diff --
You can use the following code to pad the convertedValue (this will be a noop for types that aren't padded):
ImmutableBytesWritable ptr = new ImmutableBytesWritable(convertedValue);
dataType.pad(ptr, field.getMaxLength(), field.getSortOrder());
convertedValue = ByteUtil.copyKeyBytesIfNecessary(ptr);
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36442865
--- Diff: phoenix-core/src/test/java/org/apache/phoenix/util/TenantIdByteConversionTest.java ---
@@ -0,0 +1,275 @@
+/*
+ * 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.util;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.fail;
+
+import java.sql.SQLException;
+import java.util.Collection;
+import java.util.List;
+
+import org.apache.phoenix.schema.*;
+import org.apache.phoenix.schema.types.*;
+import org.apache.phoenix.schema.RowKeySchema.RowKeySchemaBuilder;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import com.google.common.collect.Lists;
+import org.mockito.Mockito;
+
+/*Test the getTenantIdBytes method in ScanUtil*/
+@RunWith(Parameterized.class)
+public class TenantIdByteConversionTest {
--- End diff --
Nice tests! I'd add a couple more for tenantId as BINARY data type and tenantId declared as DESC
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128866480
Yeah, I understand that the feature allows TENANT_ID on data tables to be a different type. What I'm not sure about is how useful this feature would be in real life if internally we are treating tenant ids as strings in connections and system tables anyway. Why not just do the same for data tables, as well (which is how it is now implemented)? As you mentioned, every type can be converted to a string and users can do easily do that for data tables.
If we relax type constrains for data tables but not for metadata tables/connections we are making the implementation of multi-tenancy in phoenix internally inconsistent. Should be all or nothing IMHO.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36557713
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/MultiTenantTableIT.java ---
@@ -0,0 +1,140 @@
+/*
+ * 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.end2end;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.*;
+import java.util.Properties;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.Lists;
+
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.schema.TableAlreadyExistsException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class MultiTenantTableIT extends BaseClientManagedTimeIT {
--- End diff --
Excellent tests, Jeffrey. Can you rename this class to something that is more specific to the work you did? Otherwise the name of this class implies that it is a full test suite for everything related to multi-tenant tables, while there are a number of other test classes for multi-tenancy. Maybe something like TenantIdTypeIT?
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130350426
Alright @elilevine, final revisions to the tests are up. As I mentioned in my earlier post, I'm not quite sure where my work ends, but I'll be available to fix/help with anything needed for the next few days.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36560020
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/MultiTenantTableIT.java ---
@@ -0,0 +1,140 @@
+/*
+ * 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.end2end;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.sql.*;
+import java.util.Properties;
+import java.util.Collection;
+import java.util.List;
+import com.google.common.collect.Lists;
+
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.schema.TableAlreadyExistsException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class MultiTenantTableIT extends BaseClientManagedTimeIT {
+
+ private Connection regularConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ return DriverManager.getConnection(url, props);
+ }
+
+ private Connection tenantConnection(String url) throws SQLException {
+ Properties props = new Properties();
+ props.setProperty(PhoenixRuntime.CURRENT_SCN_ATTRIB, Long.toString(nextTimestamp()));
+ String tenantIdProperty = this.tenantId.replaceAll("\'", "");
+ props.setProperty(PhoenixRuntime.TENANT_ID_ATTRIB, tenantIdProperty);
+ return DriverManager.getConnection(url, props);
+ }
+
+ private final String ddl;
+ private final String dataType;
+ private final String tenantId;
+ private final String otherTenantId;
+ private final String table;
+
+ public MultiTenantTableIT( String dataType, String tenantId, String otherTenantId ) {
+ this.dataType = dataType;
+ this.tenantId = tenantId;
+ this.otherTenantId = otherTenantId;
+ String tbl = "foo" + dataType;
+ if(tbl.contains("(")){
+ tbl = tbl.substring(0, tbl.indexOf("("));
+ }
+ this.table = tbl;
+ this.ddl = "create table " + table + " (" + "tid "+ dataType + " NOT NULL," + "id INTEGER NOT NULL, \n"
+ + "val VARCHAR " + "CONSTRAINT pk PRIMARY KEY(tid, id)) \n"
+ + "MULTI_TENANT=true";
+ }
+
+ @Parameters
+ public static Collection<Object[]> data() {
+ List<Object[]> testCases = Lists.newArrayList();
+ testCases.add(new Object[] { "INTEGER", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "UNSIGNED_INT", "2147483647", "2147483646" });
+ testCases.add(new Object[] { "BIGINT", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "UNSIGNED_LONG", "9223372036854775807", "9223372036854775806" });
+ testCases.add(new Object[] { "TINYINT", "127", "126" });
+ testCases.add(new Object[] { "UNSIGNED_TINYINT", "85", "84" });
+ testCases.add(new Object[] { "SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "UNSIGNED_SMALLINT", "32767", "32766" });
+ testCases.add(new Object[] { "FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "UNSIGNED_FLOAT", "3.4028234", "3.4028232" });
+ testCases.add(new Object[] { "DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "UNSIGNED_DOUBLE", "1.7976931348623157", "1.7976931348623156" });
+ testCases.add(new Object[] { "DECIMAL", "3.402823466", "3.402823465" });
+ testCases.add(new Object[] { "VARCHAR", "\'NameOfTenant\'", "\'Nemesis\'" });
+ testCases.add(new Object[] { "CHAR(10)", "\'1234567890\'", "\'Nemesis\'" });
+
+ return testCases;
+ }
+
+ @Test
+ public void testMultiTenantTables() throws Exception {
+ Connection conn = regularConnection(getUrl());
+ conn.setAutoCommit(true);
+ conn.createStatement().execute(ddl);
+
+
+ try {
+ conn.createStatement().execute(ddl);
--- End diff --
So I think this is convention I picked up from some other tests, but the goal of that try block is to actually throw the exception and then catch it and carry on. It seemed like the way that other tests were verifying table creation was to try creating it again and verifying that the second create fails because it already exists. I do want to verify the table got created, because prior to my changes a tenantId of anything but CHAR or VARCHAR would fail at creation. If there is a better way of checking if a table exists, I would certainly be open to it though.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36560516
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/BaseQueryPlan.java ---
@@ -204,8 +204,18 @@ public final ResultIterator iterator(final List<? extends SQLCloseable> dependen
} else {
ScanUtil.setTimeRange(scan, context.getScanTimeRange());
}
-
- ScanUtil.setTenantId(scan, connection.getTenantId() == null ? null : connection.getTenantId().getBytes());
+ byte[] tenantIdBytes;
--- End diff --
I guess the question is why do we need to grab tenantIdBytes from the table (in the if {} block) now instead of just getting them from Connection.getTenantId().getBytes() like we did before?
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36559583
--- Diff: phoenix-core/src/test/java/org/apache/phoenix/util/TenantIdByteConversionTest.java ---
@@ -0,0 +1,275 @@
+/*
+ * 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.util;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.fail;
+
+import java.sql.SQLException;
+import java.util.Collection;
+import java.util.List;
+
+import org.apache.phoenix.schema.*;
+import org.apache.phoenix.schema.types.*;
+import org.apache.phoenix.schema.RowKeySchema.RowKeySchemaBuilder;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import com.google.common.collect.Lists;
+import org.mockito.Mockito;
+
+/*Test the getTenantIdBytes method in ScanUtil*/
+@RunWith(Parameterized.class)
+public class TenantIdByteConversionTest {
--- End diff --
Also, can you test non-VARCHAR tenant ids with tenant-specific secondary indexes and sequences?
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128869235
The system tables need to be able to store the tenant ID regardless of its type in the data table, so it wouldn't make sense to type the system tables (i.e. the *all* approach isn't viable IMHO). If a table already exists with a different typed leading column than CHAR/VARCHAR, then it would not be possible to map a multi-tenant Phoenix table to it. This just provides a little bit more flexibility. I'll ping the original filer of the JIRA and see if he has further comment on his use case.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36558428
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java ---
@@ -236,6 +236,7 @@ public SQLException newException(SQLExceptionInfo info) {
CANNOT_CREATE_TENANT_SPECIFIC_TABLE(1030, "42Y89", "Cannot create table for tenant-specific connection"),
DEFAULT_COLUMN_FAMILY_ONLY_ON_CREATE_TABLE(1034, "42Y93", "Default column family may only be specified when creating a table."),
INSUFFICIENT_MULTI_TENANT_COLUMNS(1040, "42Y96", "A MULTI_TENANT table must have two or more PK columns with the first column being NOT NULL and of type VARCHAR or CHAR."),
--- End diff --
Type information at the end of the error message no longer applies.
---
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-1673 Allow TenantId to be of any int...
Posted by elilevine <gi...@git.apache.org>.
Github user elilevine commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-130783527
This PR is now in master and can be closed.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-129597825
Hey @elilevine, thanks and I appreciate the review, I've been learning a lot :)
In terms of our use case, we have a pre-existing schema that we don't want to modify. For one, it would be a significant refactor to some of our other code in order to handle converting our existing TenantIds to strings.
The other issue is that we have a lot of traffic that is interacting with the cluster directly through HBase and not through Phoenix. For this reason we like having the TenantIds as fixed length, so that the rowkeys are easy and logical to parse.
It also seems reasonable to me that if the TenantId is naturally a number instead of a name that it could be represented with one, instead of forcing a conversion to string when we store it.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128498647
Added the changes requested, and removed the redundant padding.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-129874512
For points #1 and #3, I think I really mean converting the data we already have. We have a lot of data in our system with our current schema and would have to upgrade the entire schema and most of our historical data to fit the new standards, which for us would be a non-trivial process.
We are also writing to the underlying tables through our own library, not through Phoenix, and that system is set up to have TenantId as an UNSIGNED_INT. This code is pretty ingrained in our system, and would require a massive refactor and significant restructure of how our cluster works to switch over to using something like fixed-length CHAR arrays.
As for the tests, I'll get them up as soon as possible.
---
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-1673 Allow TenantId to be of any int...
Posted by JeffreyLyonsD2L <gi...@git.apache.org>.
Github user JeffreyLyonsD2L commented on the pull request:
https://github.com/apache/phoenix/pull/104#issuecomment-128835914
Thanks for all the feedback @elilevine and @samarthjain! I'm east coast, so I'm just heading out of the office now, but I should be able to get the changes up sometime tomorrow.
The above question kind of goes outside my realm of interaction with Phoenix so I'm not sure if my handle on it is correct. Is the point of the TenantId in a table like System.Catalog to tie specific schema to a tenant? If so I would lean toward leaving as VARCHAR since any TenantId has to be at least convertible to a VARCHAR since it comes down the connection as one, though I agree the your 'correct' solution seems like the most complete way forward.
---
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-1673 Allow TenantId to be of any int...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/104#discussion_r36440666
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java ---
@@ -114,6 +116,7 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Filt
boolean isMultiTenant = tenantId != null && table.isMultiTenant();
if (isMultiTenant) {
tenantId = ScanUtil.padTenantIdIfNecessary(schema, isSalted, tenantId);
--- End diff --
Take a look at ScanUtil.padTenantIdIfNecessary(), as it assumes tenantId is a string:
public static PName padTenantIdIfNecessary(RowKeySchema schema, boolean isSalted, PName tenantId) {
int pkPos = isSalted ? 1 : 0;
String tenantIdStr = tenantId.getString();
Field field = schema.getField(pkPos);
PDataType dataType = field.getDataType();
boolean isFixedWidth = dataType.isFixedWidth();
Integer maxLength = field.getMaxLength();
if (isFixedWidth && maxLength != null) {
if (tenantIdStr.length() < maxLength) {
tenantIdStr = (String)dataType.pad(tenantIdStr, maxLength);
return PNameFactory.newName(tenantIdStr);
}
}
return tenantId;
}
Probably best if your ScanUtil.getTenantIdBytes() function takes into account padding as well and we can get rid of this util function (see comment below on that).
---
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.
---