You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/01/16 00:21:09 UTC
[GitHub] [drill] cgivre opened a new pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
cgivre opened a new pull request #2428:
URL: https://github.com/apache/drill/pull/2428
# [DRILL-8108](https://issues.apache.org/jira/browse/DRILL-8108): Excel Reader Fails with Duplicate Columns
## Description
In its current implementation, if Drill encounters an Excel file which contains duplicate column names, it will fail to read the data. This PR fixes this issue by appending `_n` after the duplicate column.
## Documentation
No user facing changes.
## Testing
Added two unit tests.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] pjfanning commented on a change in pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
pjfanning commented on a change in pull request #2428:
URL: https://github.com/apache/drill/pull/2428#discussion_r785377804
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -364,6 +368,36 @@ private void getColumnHeaders(SchemaBuilder builder) {
builder.buildSchema();
}
+ /**
+ * This function verifies whether a given column name is already present in the projected schema.
+ * If so, it appends _n to the column name. N will be incremented for every duplicate column
+ * @param columnName The original column
+ * @return The deconflicted column name
+ */
+ private String deconflictColumnNames(String columnName) {
+ Pattern pattern = Pattern.compile("_(\\d+)$");
+ Matcher matcher = pattern.matcher(columnName);
+ while (containsIgnoreCase(columnName, excelFieldNames)) {
+ if (matcher.find()) {
+ int index = Integer.parseInt(matcher.group(1));
+ index++;
+ columnName = matcher.replaceFirst("_" + index);
+ } else {
+ columnName = columnName + "_1";
+ }
+ }
+ return columnName;
+ }
+
+ private boolean containsIgnoreCase(String str, List<String> list) {
Review comment:
this is a little slow - you could maintain a TreeSet which has the same elements as `excelFieldNames` - `TreeSet<String>(String.CASE_INSENSITIVE_ORDER)` -- it would be great if LinkedHashSet supported this ordering too but it seems it doesn't
So the trade off is using a little extra memory to keep the TreeSet and being about to do lookups in order (log-base2-n) time instead of order (n). Not a big deal really but worth considering.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2428:
URL: https://github.com/apache/drill/pull/2428#discussion_r785378826
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -364,6 +368,36 @@ private void getColumnHeaders(SchemaBuilder builder) {
builder.buildSchema();
}
+ /**
+ * This function verifies whether a given column name is already present in the projected schema.
+ * If so, it appends _n to the column name. N will be incremented for every duplicate column
+ * @param columnName The original column
+ * @return The deconflicted column name
+ */
+ private String deconflictColumnNames(String columnName) {
+ Pattern pattern = Pattern.compile("_(\\d+)$");
+ Matcher matcher = pattern.matcher(columnName);
+ while (containsIgnoreCase(columnName, excelFieldNames)) {
+ if (matcher.find()) {
+ int index = Integer.parseInt(matcher.group(1));
+ index++;
+ columnName = matcher.replaceFirst("_" + index);
+ } else {
+ columnName = columnName + "_1";
+ }
+ }
+ return columnName;
+ }
+
+ private boolean containsIgnoreCase(String str, List<String> list) {
Review comment:
@pjfanning 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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #2428:
URL: https://github.com/apache/drill/pull/2428
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] pjfanning commented on a change in pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
pjfanning commented on a change in pull request #2428:
URL: https://github.com/apache/drill/pull/2428#discussion_r785376059
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -364,6 +368,27 @@ private void getColumnHeaders(SchemaBuilder builder) {
builder.buildSchema();
}
+ /**
+ * This function verifies whether a given column name is already present in the projected schema.
+ * If so, it appends _n to the column name. N will be incremented for every duplicate column
+ * @param columnName The original column
+ * @return The deconflicted column name
+ */
+ private String deconflictColumnNames(String columnName) {
+ Pattern pattern = Pattern.compile("_(\\d+)$");
+ Matcher matcher = pattern.matcher(columnName);
+ while (excelFieldNames.contains(columnName)) {
Review comment:
Does case matter because `contains` is case sensitive - so if col1 is called 'Column' and col2 is called 'column' then this code won't rename col2.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2428:
URL: https://github.com/apache/drill/pull/2428#issuecomment-1013781073
@pjfanning Would you mind doing a quick review of this PR?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] pjfanning commented on a change in pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
pjfanning commented on a change in pull request #2428:
URL: https://github.com/apache/drill/pull/2428#discussion_r785376059
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -364,6 +368,27 @@ private void getColumnHeaders(SchemaBuilder builder) {
builder.buildSchema();
}
+ /**
+ * This function verifies whether a given column name is already present in the projected schema.
+ * If so, it appends _n to the column name. N will be incremented for every duplicate column
+ * @param columnName The original column
+ * @return The deconflicted column name
+ */
+ private String deconflictColumnNames(String columnName) {
+ Pattern pattern = Pattern.compile("_(\\d+)$");
+ Matcher matcher = pattern.matcher(columnName);
+ while (excelFieldNames.contains(columnName)) {
Review comment:
Does case matter because `contains` is case sensitive? - so if col1 is called 'Column' and col2 is called 'column' then this code won't rename col2.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2428: DRILL-8108: Excel Reader Fails with Duplicate Columns
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2428:
URL: https://github.com/apache/drill/pull/2428#discussion_r785376820
##########
File path: contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelBatchReader.java
##########
@@ -364,6 +368,27 @@ private void getColumnHeaders(SchemaBuilder builder) {
builder.buildSchema();
}
+ /**
+ * This function verifies whether a given column name is already present in the projected schema.
+ * If so, it appends _n to the column name. N will be incremented for every duplicate column
+ * @param columnName The original column
+ * @return The deconflicted column name
+ */
+ private String deconflictColumnNames(String columnName) {
+ Pattern pattern = Pattern.compile("_(\\d+)$");
+ Matcher matcher = pattern.matcher(columnName);
+ while (excelFieldNames.contains(columnName)) {
Review comment:
@pjfanning
Good catch. It turns out it does matter... fixed and updated UT.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org