You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by blrunner <gi...@git.apache.org> on 2015/07/25 06:17:09 UTC
[GitHub] tajo pull request: TAJO-1675: NPE when selecting data from informa...
GitHub user blrunner opened a pull request:
https://github.com/apache/tajo/pull/653
TAJO-1675: NPE when selecting data from information_schema.partition_keys
I added getAllPartitionKeys to CatalogStore and added some codes to NonForwardQueryResultSystemScanner. If you test it on tsql, you never reproduce NPE. But you can get any rows from partition_keys table because current tajo doesn't provide loading dynamic partitions by using insert query and CTAS query. You will get partition keys from information schema after resolving https://github.com/apache/tajo/pull/630 .
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/blrunner/tajo TAJO-1675
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/653.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 #653
----
commit 7572f6d71ba2dff9010e662b512068b6b4027c87
Author: JaeHwa Jung <bl...@apache.org>
Date: 2015-07-25T04:11:23Z
TAJO-1675: NPE when selecting data from information_schema.partition_keys.
----
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-135280383
@jihoonson
Thank your for your kind reply. I've updated the patch using the first choice. :)
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-134777864
@blrunner, thanks for update. I left a comment, but I know that my comment is not directly related to your patch. You may have two choices. One is updating the patch to improve the interface design, and the other is booking it as another issue in Jira. If you choose the second option, it should be resolved before our upcoming release, I think.
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37429580
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -2277,6 +2279,41 @@ public void addPartitions(String databaseName, String tableName, List<CatalogPro
CatalogUtil.closeQuietly(pstmt4);
}
}
+ @Override
+ public List<TablePartitionKeyProto> getAllPartitionKeys() {
+ Connection conn = null;
+ Statement stmt = null;
+ ResultSet resultSet = null;
+
+ List<TablePartitionKeyProto> partitions = new ArrayList<TablePartitionKeyProto>();
+
+ try {
+ String sql = " SELECT A." + COL_PARTITIONS_PK + ", A.COLUMN_NAME, A.PARTITION_VALUE " +
+ " FROM " + TB_PARTTION_KEYS + " A " +
+ " WHERE A." + COL_PARTITIONS_PK + " > 0 "+
+ " AND A.COLUMN_NAME IS NOT NULL " +
--- End diff --
```COLUMN_NAME``` is already defined with ```NOT NULL```.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37735956
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/master/TestNonForwardQueryResultSystemScanner.java ---
@@ -18,9 +18,22 @@
package org.apache.tajo.master;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
--- End diff --
Unused imports.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-131990167
Hi @jihoonson
Thank you for your detailed review.
I've just updated it using your comments.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r36603952
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -152,6 +152,12 @@ message TablePartitionProto {
optional string path = 4;
}
+message TablePartitionKeysProto {
--- End diff --
A message represents a single partition. So, ```TablePartitionKeyProto``` looks to be more proper.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-129324606
@blrunner, thanks for your work. Please consider my comments.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37735332
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
--- End diff --
Path also looks mandatory field. Is there any case that path is null?
If not, it would be great if you also change it to ```required```.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37735836
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
- optional int32 id = 4;
+ optional int32 partition_id = 4;
+ optional int32 tid = 5;
}
message PartitionKeyProto {
required string columnName = 1;
optional string parentColumnName = 2;
--- End diff --
parentColumnName is unused. Please remove 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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37736825
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/master/TestNonForwardQueryResultSystemScanner.java ---
@@ -38,4 +51,89 @@ public void testGetNextRowsForTable() throws Exception {
public void testGetClusterDetails() throws Exception {
assertQueryStr("SELECT TYPE FROM INFORMATION_SCHEMA.CLUSTER");
}
+
+ @Test
+ public final void testGetInformationSchema() throws Exception {
+ executeDDL("create_partitioned_table.sql", null);
+
+ String simpleTableName = "information_schema_test_table";
+ String tableName = CatalogUtil.buildFQName(getCurrentDatabase(), simpleTableName);
+ assertTrue(catalog.existsTable(tableName));
+
+ TableDesc retrieved = catalog.getTableDesc(tableName);
+
+ executeDDL("alter_table_add_partition1.sql", null);
+ executeDDL("alter_table_add_partition2.sql", null);
+
+ List<CatalogProtos.DatabaseProto> allDatabases = catalog.getAllDatabases();
+ int dbId = -1;
+ for (CatalogProtos.DatabaseProto database : allDatabases) {
+ if (database.getName().equals(getCurrentDatabase())) {
+ dbId = database.getId();
+ }
+ }
+ assertNotEquals(dbId, -1);
+
+ int tableId = -1;
+ List<CatalogProtos.TableDescriptorProto> allTables = catalog.getAllTables();
+ for(CatalogProtos.TableDescriptorProto table : allTables) {
+ if (table.getDbId() == dbId && table.getName().equals(simpleTableName)) {
+ tableId = table.getTid();
+ }
+ }
+ assertNotEquals(tableId, -1);
+
+ List<CatalogProtos.PartitionDescProto> allPartitions = catalog.getAllPartitions();
+ List<CatalogProtos.PartitionDescProto> resultPartitions = TUtil.newList();
+
+ int partitionId = 0;
+ for (CatalogProtos.PartitionDescProto partition : allPartitions) {
+ if (partition.getTid() == tableId
+ && partition.getPartitionName().equals("col3=1/col4=2")
+ && partition.getPath().equals(retrieved.getUri().toString() + "/col3=1/col4=2")
+ ){
+ resultPartitions.add(partition);
+ partitionId = partition.getPartitionId();
+ }
+ }
+ assertEquals(resultPartitions.size(), 1);
+ assertEquals(resultPartitions.get(0).getPartitionName(), "col3=1/col4=2");
+
+ List<CatalogProtos.PartitionKeyProto> tablePartitionKeys = catalog.getAllPartitionKeys();
+ List<CatalogProtos.PartitionKeyProto> resultPartitionKeys = TUtil.newList();
+
+ for (CatalogProtos.PartitionKeyProto partitionKey: tablePartitionKeys) {
+ if (partitionKey.getPartitionId() == partitionId
+ && (partitionKey.getColumnName().equals("col3") && partitionKey.getPartitionValue().equals("1")
+ || partitionKey.getColumnName().equals("col4") && partitionKey.getPartitionValue().equals("2"))) {
+ resultPartitionKeys.add(partitionKey);
+ }
+ }
+ assertEquals(resultPartitionKeys.size(), 2);
+ assertEquals(resultPartitionKeys.get(0).getColumnName(), "col3");
+ assertEquals(resultPartitionKeys.get(0).getPartitionValue(), "1");
+ assertEquals(resultPartitionKeys.get(1).getColumnName(), "col4");
+ assertEquals(resultPartitionKeys.get(1).getPartitionValue(), "2");
--- End diff --
Above tests looks duplicate with below tests. Only difference is that above tests use catalog client APIs while below tests use tajo client APIs. For consistency, I prefer to use tajo client APIs in this test class.
In addition, I think that we need to test the case when information of a lot of partitions and partition keys is stored in catalog. So, it would be great if more partitions are added when initializing.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37430317
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -152,6 +152,12 @@ message TablePartitionProto {
optional string path = 4;
}
+message TablePartitionKeyProto {
--- End diff --
How is it different from ```PartitionKeyProto```?
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r36605076
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java ---
@@ -579,10 +578,19 @@ public void dropPartitionMethod(String databaseName, String tableName) throws Ca
}
public List<TablePartitionProto> getAllPartitions() throws CatalogException {
+ int tableId = 0, partitionId = 0;
+ List<TableDescriptorProto> tables = getAllTables();
List<TablePartitionProto> protos = new ArrayList<TablePartitionProto>();
- Set<String> tables = partitions.keySet();
- for (String table : tables) {
- Map<String, CatalogProtos.PartitionDescProto> entryMap = partitions.get(table);
+
+ Set<String> partitionTables = partitions.keySet();
+ for (String partitionTable : partitionTables) {
+ for (TableDescriptorProto table : tables) {
--- End diff --
Since it is guaranteed that the tables corresponding to partitions always exist, it looks not necessary to match table names.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r36604876
--- Diff: tajo-core/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java ---
@@ -103,10 +107,79 @@ public final void testAlterTableAddPartition() throws Exception {
assertTrue(fs.exists(partitionPath));
assertTrue(partitionPath.toString().indexOf("col3=1/col4=2") > 0);
+ List<CatalogProtos.DatabaseProto> allDatabases = catalog.getAllDatabases();
--- End diff --
Please move this test to ```TestNonForwardQueryResultSystemScanner```.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner closed the pull request at:
https://github.com/apache/tajo/pull/653
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37735887
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java ---
@@ -25,6 +25,7 @@
import org.apache.tajo.catalog.CatalogUtil;
import org.apache.tajo.catalog.TableDesc;
import org.apache.tajo.catalog.proto.CatalogProtos;
+import org.apache.tajo.util.TUtil;
--- End diff --
Unused import.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37428677
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -2138,8 +2138,10 @@ private void setPartitionKeys(int pid, PartitionDescProto.Builder partitionDesc)
List<TablePartitionProto> partitions = new ArrayList<TablePartitionProto>();
try {
- String sql = "SELECT " + COL_PARTITIONS_PK + ", " + COL_TABLES_PK + ", PARTITION_NAME, " +
- " PATH FROM " + TB_PARTTIONS;
+ String sql = "SELECT A." + COL_PARTITIONS_PK + ", A." + COL_TABLES_PK + ", A.PARTITION_NAME, A.PATH "
+ + " FROM " + TB_PARTTIONS + " A "
+ + " WHERE A." + COL_PARTITIONS_PK + " > 0 "
+ + " AND A." + COL_TABLES_PK + " > 0 ";
--- End diff --
```COL_PARTITIONS_PK``` and ```COL_TABLES_PK``` are primary keys and thus cannot have negative values.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-138427235
Selecting partition keys is not necessary because partitions are searched with its partition name. It would be better to remove this function.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37429239
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -2277,6 +2279,41 @@ public void addPartitions(String databaseName, String tableName, List<CatalogPro
CatalogUtil.closeQuietly(pstmt4);
}
}
+ @Override
+ public List<TablePartitionKeyProto> getAllPartitionKeys() {
+ Connection conn = null;
+ Statement stmt = null;
+ ResultSet resultSet = null;
+
+ List<TablePartitionKeyProto> partitions = new ArrayList<TablePartitionKeyProto>();
+
+ try {
+ String sql = " SELECT A." + COL_PARTITIONS_PK + ", A.COLUMN_NAME, A.PARTITION_VALUE " +
+ " FROM " + TB_PARTTION_KEYS + " A " +
+ " WHERE A." + COL_PARTITIONS_PK + " > 0 "+
--- End diff --
What does ```COL_PARTITIONS_PK > 0``` mean?
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37735899
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java ---
@@ -35,6 +36,7 @@
@Category(IntegrationTest.class)
public class TestAlterTable extends QueryTestCaseBase {
+
--- End diff --
Meaningless change.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-138434742
@jihoonson
Okay, I'll remove it from this function at another issue.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-137657967
Sorry @jihoonson
Honestly, I had made unnecessary adjustments to catalog protocol. As a result, we lost unnecessary times. So, I've removed unnecessary modification.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37430341
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/MemStore.java ---
@@ -598,6 +598,27 @@ public boolean existPartitionMethod(String databaseName, String tableName)
return protos;
}
+ public List<TablePartitionKeyProto> getAllPartitionKeys() {
+ List<TablePartitionKeyProto> protos = new ArrayList<TablePartitionKeyProto>();
+ Set<String> partitionTables = partitions.keySet();
+ for (String partitionTable : partitionTables) {
+ Map<String, CatalogProtos.PartitionDescProto> entryMap = partitions.get(partitionTable);
+ for (Map.Entry<String, CatalogProtos.PartitionDescProto> proto : entryMap.entrySet()) {
+ CatalogProtos.PartitionDescProto partitionDescProto = proto.getValue();
+
+ for (PartitionKeyProto partitionKey : partitionDescProto.getPartitionKeysList()) {
+ TablePartitionKeyProto.Builder builder = TablePartitionKeyProto.newBuilder();
+ builder.setColumnName(partitionKey.getColumnName());
+ builder.setPartitionValue(partitionKey.getPartitionValue());
+ builder.setPartitionId(0);
--- End diff --
partition id is always 0.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37429665
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -2277,6 +2279,41 @@ public void addPartitions(String databaseName, String tableName, List<CatalogPro
CatalogUtil.closeQuietly(pstmt4);
}
}
+ @Override
+ public List<TablePartitionKeyProto> getAllPartitionKeys() {
+ Connection conn = null;
+ Statement stmt = null;
+ ResultSet resultSet = null;
+
+ List<TablePartitionKeyProto> partitions = new ArrayList<TablePartitionKeyProto>();
+
+ try {
+ String sql = " SELECT A." + COL_PARTITIONS_PK + ", A.COLUMN_NAME, A.PARTITION_VALUE " +
+ " FROM " + TB_PARTTION_KEYS + " A " +
+ " WHERE A." + COL_PARTITIONS_PK + " > 0 "+
+ " AND A.COLUMN_NAME IS NOT NULL " +
+ " AND A.PARTITION_VALUE IS NOT NULL ";
--- End diff --
Why are partition keys filtered if they have null partition value?
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37734751
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
- optional int32 id = 4;
+ optional int32 partition_id = 4;
--- End diff --
Partition id is the primary key of the ```partitions``` table. Why is it optional?
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37935397
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
- optional int32 id = 4;
+ optional int32 partition_id = 4;
--- End diff --
This interface is very weird because it involves many potential bugs. We must handle the cases that some mandatory fields are missed, and this is naturally done by protocol buffer. The below problem of optionally declared path is also related to this weird design.
A better design is as follows. PartitionDescProto corresponds to PartitionDesc, and should be used to represent a partition as TableDesc represents a table.
To do so, PartitionDescProto should be used as only the result of the methods which retrieves partitions. For input params, a new data structure is required.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37489581
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -152,6 +152,12 @@ message TablePartitionProto {
optional string path = 4;
}
+message TablePartitionKeyProto {
--- End diff --
PartitionKeyProto already exists.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r36604993
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -2259,6 +2261,41 @@ public void addPartitions(String databaseName, String tableName, List<CatalogPro
CatalogUtil.closeQuietly(pstmt4);
}
}
+ @Override
+ public List<TablePartitionKeysProto> getAllPartitionKeys() throws CatalogException {
+ Connection conn = null;
+ Statement stmt = null;
+ ResultSet resultSet = null;
+
+ List<TablePartitionKeysProto> partitions = new ArrayList<TablePartitionKeysProto>();
+
+ try {
+ String sql = " SELECT C." + COL_PARTITIONS_PK + ", C.COLUMN_NAME, C.PARTITION_VALUE " +
+ " FROM " + TB_TABLES + " A " +
+ " JOIN " + TB_PARTTIONS + " B ON A." + COL_TABLES_PK + " = B." + COL_TABLES_PK
--- End diff --
Since it is guaranteed that the tables corresponding to partitions always exist, join looks not necessary.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37429884
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -152,6 +152,12 @@ message TablePartitionProto {
optional string path = 4;
}
+message TablePartitionKeyProto {
+ required int32 partition_id = 1;
+ required string columnName = 2;
+ required string partitionValue = 3;
--- End diff --
Partition value can be null.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37490911
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -152,6 +152,12 @@ message TablePartitionProto {
optional string path = 4;
}
+message TablePartitionKeyProto {
--- End diff --
Yes. I mean, what is the difference between PartitionKeyProto and TablePartitionKeyProto? Their name is similar, but contents are different. I wonder their proper usage.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r36604569
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java ---
@@ -1230,6 +1230,29 @@ public ReturnState addPartitions(RpcController controller, AddPartitionsProto re
}
@Override
+ public GetTablePartitionKeysResponse getAllPartitionKeys(RpcController controller,
+ NullProto request) throws ServiceException {
--- End diff --
Please fix indent.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-132910321
Thanks @jihoonson
A little while ago, as a fix again. :)
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37427089
--- Diff: tajo-core-tests/src/test/java/org/apache/tajo/engine/query/TestAlterTable.java ---
@@ -103,10 +107,33 @@ public final void testAlterTableAddPartition() throws Exception {
assertTrue(fs.exists(partitionPath));
assertTrue(partitionPath.toString().indexOf("col3=1/col4=2") > 0);
+ boolean existPartition = false;
+ List<CatalogProtos.TablePartitionProto> allPartitions = catalog.getAllPartitions();
--- End diff --
As I said before, this test should be moved to TestNonForwardQueryResultSystemScanner. (Maybe this class is also renamed, for example to TestInformationSchema). Here are some reasons.
First, this class is for testing alter table operation rather than functionality of information schema. Second, in this implementation, several tests are mixed together, so it is difficult to figure out which lines test which functionality. Unit testing is to test the functionality of individual modules.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37882522
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
- optional int32 id = 4;
+ optional int32 partition_id = 4;
+ optional int32 tid = 5;
--- End diff --
Please see above comments.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37427851
--- Diff: tajo-catalog/tajo-catalog-client/src/main/proto/CatalogProtocol.proto ---
@@ -89,6 +89,11 @@ message GetTablePartitionsResponse {
repeated TablePartitionProto part = 2;
}
+message GetTablePartitionKeysResponse {
+ required ReturnState state = 1;
+ repeated TablePartitionKeyProto partKey = 2;
--- End diff --
```partitionKey``` looks more intuitive.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37428381
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/CatalogServer.java ---
@@ -1044,6 +1044,29 @@ public ReturnState addPartitions(RpcController controller, AddPartitionsProto re
}
@Override
+ public GetTablePartitionKeysResponse getAllPartitionKeys(RpcController controller, NullProto request) throws
+ ServiceException {
--- End diff --
Please fix the indent between ```throws``` and ```ServiceException```.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37735085
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
- optional int32 id = 4;
+ optional int32 partition_id = 4;
+ optional int32 tid = 5;
--- End diff --
tid is also mandatory field.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r36604991
--- Diff: tajo-catalog/tajo-catalog-server/src/main/java/org/apache/tajo/catalog/store/AbstractDBStore.java ---
@@ -2122,8 +2122,10 @@ private void setPartitionKeys(int pid, PartitionDescProto.Builder partitionDesc)
List<TablePartitionProto> partitions = new ArrayList<TablePartitionProto>();
try {
- String sql = "SELECT " + COL_PARTITIONS_PK + ", " + COL_TABLES_PK + ", PARTITION_NAME, " +
- " PATH FROM " + TB_PARTTIONS;
+ String sql = "SELECT A." + COL_PARTITIONS_PK + ", A." + COL_TABLES_PK + ", A.PARTITION_NAME, " +
+ " A.PATH FROM " + TB_PARTTIONS + " A JOIN " + TB_TABLES + " B "
--- End diff --
Since it is guaranteed that the tables corresponding to partitions always exist, join looks not necessary.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37881184
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
--- End diff --
When adding or dropping partition, CatalogClient calls alter table API with AlterTableDescProto. and the proto maintains partition informations to PartitionDescProto. When adding partition, the partition path will be not null. But when dropping partition, the path will be null because Tajo can delete partition informs in CatalogStore with partition name.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on a diff in the pull request:
https://github.com/apache/tajo/pull/653#discussion_r37882457
--- Diff: tajo-catalog/tajo-catalog-common/src/main/proto/CatalogProtos.proto ---
@@ -247,13 +240,15 @@ message PartitionDescProto {
required string partitionName = 1;
repeated PartitionKeyProto partitionKeys = 2;
optional string path = 3;
- optional int32 id = 4;
+ optional int32 partition_id = 4;
--- End diff --
PartitionDescProto is used for various catalog API, for examples, output parameter of getPartitionByPartitionName and getPartitionsByTableName and getAllPartitions, input parameter of alterTable and addPartitions. In case of output parameter, partition id always is not null. But in case of input parameter, partition id will be null because CatalogClient can get partition informs with table name and partition name.
---
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] tajo pull request: TAJO-1675: NPE when selecting data from informa...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/653#issuecomment-134656287
Thanks @jihoonson
I updated the patch using your comments and leaved some comments about your questions.
---
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.
---