You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by GitBox <gi...@apache.org> on 2020/07/16 14:54:56 UTC
[GitHub] [openjpa] eolivelli opened a new pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
eolivelli opened a new pull request #69:
URL: https://github.com/apache/openjpa/pull/69
While working at HerdDBDictionary I figured out that integration tests do not work because of two problems:
- a bug in SelectImpl#getColumnAlias, in which we are not applying delimiters to the column name
- SchemaTool does not recognize that a column is already present on a table, in case of delimitedCase = SCHEMA_CASE_LOWER and schemaCase = SCHEMA_CASE_LOWER
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-659466960
I will be happy to add tests in case the direction of this patch is good
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660335970
now I see why you asked for 'install'
the ant plugin needs the jars into the maven repository
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660928008
Once double allocation (https://github.com/apache/openjpa/pull/69/files#diff-59344a422210f668cbeb574dc22e8339R324) is fixed it can be merged for me
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456493228
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
_colMap
.values()
.stream()
.map(Column::getIdentifier)
.toArray(DBIdentifier[]::new);
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-659467892
@eolivelli looks "expected" to me but it needs harnessing for both changes too IMHO
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456512187
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
Proposal was just to not to toList().toArray which allocates twice for no real reason the same structure. Probably missed a .map in my snippet but the double allocation should be avoided IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660127090
@eolivelli it is ok to fix herddb dict there since it is fully related IMHO. The first part of the diff is suspicious (schematool diff seems unnecessary and getColumnNames change is unlikely too). Would also be neat to not drop delimiters if the dictionnary does not use them (normalizeColumnKey). If it helps I think we can add a flag in DBIdentifier to notify it is delimited or not and have both the delimited and not delimited values (raw name vs statement name).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli removed a comment on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli removed a comment on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660335970
now I see why you asked for 'install'
the ant plugin needs the jars into the maven repository
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660335524
all tests passed locally
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli edited a comment on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-661834012
@ilgrosso or @rmannibucau can you please merge this patch ?
I am going to work on the follow up work about storing into DBIdentifier a property that tells if the id has been already delimited
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456520863
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
Hmm, maybe diff is not up to date but you still allocate 2 arrays, first one is a DBIdentifier array instead of an ArrayList but it is ~the same size (I'm thinking to the case we have > 200 columns in the db and it happens) so I would just do:
_colMap
.values()
.stream()
.map(Column::getIdentifier)
.map(DBIdentifier::getName)
.toArray(String[]::new);
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456882860
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
@rmannibucau I have created https://issues.apache.org/jira/browse/OPENJPA-2820 for this follow up work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456514057
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
yes I understood and I have updated the code, thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-661843497
@ilgrosso thanks to you guys for providing OpenJPA to the public
starting on https://issues.apache.org/jira/browse/OPENJPA-2820 soon
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] ilgrosso merged pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
ilgrosso merged pull request #69:
URL: https://github.com/apache/openjpa/pull/69
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-661834012
@ilgrosso or @rmannibucau can you please merge this patch.
I am going to work on the follow up work about storing into DBIdentifier a property that tells if the id has been already delimited
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-661005910
Mergeable for me (i cant do it right now so if any dev with the perms agree and comes feel free to hit the green button)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456499123
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
@rmannibucau
I can't get your suggestion
If I change this to:
```
_colMap
.values()
.stream()
.map(Column::getIdentifier)
.toArray(DBIdentifier[]::new);
```
I get a DBIdentifier[] and not a String[]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456500957
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
> Makes sense to me to know if the identifier had been quoted or not and does not require a single signature change in methods
I would like this change but it is not trivial, as I see that sometimes we are manipulating the internal `_name` of DBIdentifier, and tracking when it is already delimited or not seems to me an hard task.
It can be done in a follow up patch. I will be happy to follow up in this direction
I am trying now to reduce the overhead of removeDelimiters, it does unnecessary memory allocations
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456494234
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
the identifier should always (i know a few cases where it is not the case but shouldnt impact you or means it must be fixed) be created from the dict so just inject that state in the identifier? Makes sense to me to know if the identifier had been quoted or not and does not require a single signature change in methods.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456480105
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
@rmannibucau about applying removeDelimiters only when needed:
my original fix was to handle this only in SchemaTool, but just adding the new test case showed that the situation is more complex.
Table class does not have a reference to the DBDictionary and we can't change the signature of all methods, it is kind of Public API that can be overridden by custom DBDictionary implementations
Tests are also failing if you switch to `DBIdentifier.toUpper(DBIdentifier.removeDelimiters(name), true);`
IMHO we could also probably clean up some of those methods in order to reduce useless allocation, but it can be a separate follow up patch.
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
`_colMap.keySet()` contains an internal representation of column names (all uppercased)
this method is used only for error messages and IMHO it is better to have a consistent handling of this internal data structure
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/SchemaTool.java
##########
@@ -1030,10 +1032,7 @@ private void drop(SchemaGroup db, SchemaGroup repos, boolean considerDatabaseSta
continue;
if (dropColumn(cols[k])) {
- if (dbTable != null)
Review comment:
dbTable is always not null here, we have a null check 4 lines above,
I can revert if you want
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456519302
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
It is not really about the overhead (should be a "contains" in terms of complexity for most cases) but more about being fully deterministic and not "random".
Why is it hard?
Tables can be created in:
- openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/DBDictionary.java#newTable
- openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/DBDictionary.java#getValidColumnName
- org.apache.openjpa.jdbc.schema.Schema#addTable
- org.apache.openjpa.jdbc.schema.SchemaGroup#newTable
Now if you take 1 more level you always have the dictionnary handy so if simpler we can just add the dict in the table.
Alternative I had in mind was just to handle it in org.apache.openjpa.jdbc.sql.DBDictionary#getValidColumnName(org.apache.openjpa.jdbc.identifier.DBIdentifier, org.apache.openjpa.jdbc.schema.Table, boolean) in herddb dict and set the flag there.
It is not about tracking it is delimited or not but tracking that the dict is in delimited mode or not, then you would do
if identifier.delimited then removeDelimited() else noop.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456501604
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] ilgrosso commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-661835241
@eolivelli done!
Thanks for your contribution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456759854
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -312,11 +315,20 @@ public String getResourceName() {
return _rels;
}
+ /**
+ * Return the list of column names, used only for informative (error) messages.
+ * @return
+ */
public String[] getColumnNames() {
if (_colMap == null) {
return new String[0];
}
- DBIdentifier[] sNames = _colMap.keySet().toArray(new DBIdentifier[_colMap.size()]);
Review comment:
Still a double alloc no?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660118706
@rmannibucau the patch became bigger as soon as I tried to add test cases.
Still it is not complete, I have to add a test case of the SelectImpl#getColumnAlias case
This patch also is fixing HerdDBDictionary, if you prefer I can separate that change in HerdDBDictionary ticket
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660626017
all tests are passing locally on this branch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] ilgrosso commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-661011182
whoever and does the merge, please do "squash & merge" :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660941112
@rmannibucau let's see now github validation in action
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456519781
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/SelectImpl.java
##########
@@ -2711,7 +2711,7 @@ private String getColumnAlias(Column col, PathJoins pj) {
return alias + "_" + col;
}
alias = SelectImpl.toAlias(_sel.getTableIndex(col.getTable(), pj, false));
- return (alias == null) ? null : alias + "." + col;
+ return (alias == null) ? null : alias + "." + _sel._dict.getNamingUtil().toDBName(col.toString());
Review comment:
you can likely create an assert on the select string directly - even using internals to instantiate it if it helps.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456500957
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##########
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
return getColumn(DBIdentifier.newIdentifier(name, DBIdentifierType.COLUMN, true));
}
- public Column getColumn(DBIdentifier name) {
+ private Column internalGetColumn(DBIdentifier name) {
if (DBIdentifier.isNull(name) || _colMap == null)
return null;
- return _colMap.get(DBIdentifier.toUpper(name));
+ DBIdentifier key = normalizeColumnKey(name);
+ return _colMap.get(key);
+ }
+
+ public Column getColumn(DBIdentifier name) {
+ return internalGetColumn(name);
+ }
+
+ private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+ return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));
Review comment:
> Makes sense to me to know if the identifier had been quoted or not and does not require a single signature change in methods
I would like this change but it is not trivial, as I see that sometimes we are manipulating the internal `_name` of DBIdentifier, and tracking when it is already delimited or not seems to me an hard task.
It can be done in a follow up patch. I will be happy to follow up in this direction
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] rmannibucau commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660441330
Except the double alloc it looks good to me, @struberg want to have a quick look before we merge?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456502732
##########
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/SelectImpl.java
##########
@@ -2711,7 +2711,7 @@ private String getColumnAlias(Column col, PathJoins pj) {
return alias + "_" + col;
}
alias = SelectImpl.toAlias(_sel.getTableIndex(col.getTable(), pj, false));
- return (alias == null) ? null : alias + "." + col;
+ return (alias == null) ? null : alias + "." + _sel._dict.getNamingUtil().toDBName(col.toString());
Review comment:
the new test exercise this line.
I must be honest, with Derby it does not fail the test, even without this line, but without this change the integration tests with HerdDB fail
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org