You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by GerardDellemann <gi...@git.apache.org> on 2018/08/20 12:04:25 UTC
[GitHub] metamodel pull request #185: Don't support primary keys with Hive
GitHub user GerardDellemann opened a pull request:
https://github.com/apache/metamodel/pull/185
Don't support primary keys with Hive
Hive doesn't support primary keys. This PR makes sure, that when a column is defined as a primary key, then this is ignored in the create table statement.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/GerardDellemann/metamodel feature/dont-support-primary-key-with-hive
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/185.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 #185
----
commit 55f3e6dfe72ccb86b23813946693daffb84a97cd
Author: Gerard Dellemann <g....@...>
Date: 2018-08-20T12:02:24Z
Don't support primary keys with Hive
----
---
[GitHub] metamodel pull request #185: Don't support primary keys with Hive
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/185#discussion_r211947345
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcCreateTableBuilder.java ---
@@ -115,21 +115,23 @@ private String createSqlStatement(Table table) {
sb.append(" NOT NULL");
}
}
- boolean primaryKeyExists = false;
- for (int i = 0; i < columns.size(); i++) {
- if (columns.get(i).isPrimaryKey()) {
- if (!primaryKeyExists) {
- sb.append(", PRIMARY KEY(");
- sb.append(columns.get(i).getName());
- primaryKeyExists = true;
- } else {
- sb.append(",");
- sb.append(columns.get(i).getName());
+ if (queryRewriter.isPrimaryKeySupported()) {
+ boolean primaryKeyExists = false;
--- End diff --
Not completely related to this pull request, but something that we can address in this context anyway, because the added if statement here only adds to the cognitive complexity of this method. Lines 119 through 134 are a carbon copy of lines 130 through 145 from the super class `AbstractTableCreationBuilder`. So maybe it's an option to extract that code into a separate method on the `AbstractTableCreationBuilder`, which can be invoked here too, this both removes the copy/pasted code and reduces the cognitive complexity of this method.
---
[GitHub] metamodel issue #185: METAMODEL-1194 Don't support primary keys with Hive
Posted by arjansh <gi...@git.apache.org>.
Github user arjansh commented on the issue:
https://github.com/apache/metamodel/pull/185
@kaspersorensen LGTM too then.
---
[GitHub] metamodel issue #185: Don't support primary keys with Hive
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/185
And finally a small request: Can you please create a JIRA issue for this improvement. That makes the life of the committer easier so that we (or you) can add a referenced entry into CHANGES.md when it is merged.
---
[GitHub] metamodel pull request #185: METAMODEL-1194 Don't support primary keys with ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/185
---
[GitHub] metamodel issue #185: Don't support primary keys with Hive
Posted by GerardDellemann <gi...@git.apache.org>.
Github user GerardDellemann commented on the issue:
https://github.com/apache/metamodel/pull/185
Made the JIRA Issue: https://issues.apache.org/jira/browse/METAMODEL-1194
---
[GitHub] metamodel pull request #185: Don't support primary keys with Hive
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/185#discussion_r212811641
--- Diff: jdbc/src/main/java/org/apache/metamodel/jdbc/JdbcCreateTableBuilder.java ---
@@ -115,21 +115,23 @@ private String createSqlStatement(Table table) {
sb.append(" NOT NULL");
}
}
- boolean primaryKeyExists = false;
- for (int i = 0; i < columns.size(); i++) {
- if (columns.get(i).isPrimaryKey()) {
- if (!primaryKeyExists) {
- sb.append(", PRIMARY KEY(");
- sb.append(columns.get(i).getName());
- primaryKeyExists = true;
- } else {
- sb.append(",");
- sb.append(columns.get(i).getName());
+ if (queryRewriter.isPrimaryKeySupported()) {
+ boolean primaryKeyExists = false;
--- End diff --
I don't think it's that easy unfortunately. The abstract class is in the `core` module so it does not have access to `IQueryRewriter` which is a `jdbc` module specific interface. The purpose in the abstract class is really just to have a string (SQL) representation of the operation, where as here it is similar, but that string representation will actually be used to perform the operation with the JDBC driver.
---
[GitHub] metamodel issue #185: METAMODEL-1194 Don't support primary keys with Hive
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/185
OK I'll merge then :-)
---
[GitHub] metamodel issue #185: METAMODEL-1194 Don't support primary keys with Hive
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/185
LGTM. What about you @arjansh ?
---