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 2020/04/13 03:03:05 UTC
[GitHub] [drill] paul-rogers opened a new pull request #2054: DRILL-6168:
Revise format plugin table functions
paul-rogers opened a new pull request #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054
# [DRILL-6168](https://issues.apache.org/jira/browse/DRILL-6168): Revise format plugin table functions
## Description
[DRILL-7612](https://issues.apache.org/jira/browse/DRILL-7612) notes a conflict with how we handle the properties of format plugins. This PR fixes that conflict.
On the one hand, we want all plugins (storage and format) to be immutable as they are used as keys in internal maps. Those internal maps will become invalid if we change the values of any plugin property used to compute the hash value.
On the other hand, format plugins participate in table functions. The table function implementation is based on Java introspection: it creates an empty instance, then sets public fields to the values requested in the table function.
Clearly, these two use cases conflict: one requires immutability, the other mutability. The solution is to revise table functions to not require mutability. We observe the format plugins are JSON-serializable by definition. Jackson allows serializing an object to and from a generic JSON object. This becomes the foundation for the new implementation. A side benefit is that we can start with the existing plugin config values (DRILL-6168):
* If an existing format plugin is available, convert it to a generic JSON object, else create an empty object.
* Apply each table function parameter as a key/value pair to this object, overwriting properties obtained above.
* Convert the resulting object into a new format plugin config object.
With this change, we can now change every format plugin to make fields immutable. This requires adding a `@JsonCreator` constructor and sometimes "getter" functions. Also used this opportunity to clean up `equals()` and `hashCode()` functions, and add `toString()` where missing.
Doing this work, discovered that the `bootstrap-storage-plugins.json` file used the wrong (old) field name for the text format `fieldDelimiter` field: `delimiter`. Updated the file with the modern name, and added an alias to ensure the existing plugins will continue to load.
## Documentation
None for end users.
For developers, we must revise descriptions which say that fields should be mutable and public.
## Testing
Reran all unit tests to verify no regressions.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers edited a comment on issue #2054: DRILL-6168:
Revise format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers edited a comment on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615934230
@arina-ielchiieva, yes, we can modify the queries to force the incorrect behavior. We could add a yet another option to turn off inheritance and keep the old behavior. However, I'd argue that the old results were actually wrong, and the new results are correct. So, if we modify anything, it seems the best approach is to modify the expected results to reflect the correct behavior, and let the old versions fail because they were, in fact, wrong.
Actually, it is possible to modify the query to produce the correct results on old versions: we just specify values for the fields which are inherited in Drill 1.18. Doing so will mask the old, incorrect, behavior, and won't test the new behavior.
Yet another option is to modify the framework. In addition to the `.q`, and `.e` files, add a `.p` (parameters) file and the supporting code. For example:
```
validTo: "1.17"
```
For the old queries, and:
```
validFrom: "1.18"
```
For the new queries. Better would be to use commit IDs, but those are not ordered.
We used to have this problem all the time back when Drill was taking shape. We never did solve it: we just updated the tests and moved on.
Thoughts?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #2054: DRILL-6168: Revise
format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615470669
A similar analysis applies to the other queries. For `drill-3149_1`:
```
select columns[0] from table(`table_function/cr_lf.csv`(type=>'text', lineDelimiter=>'\r\n'))
```
Expected (confusing: each line is one field, `columns[0]`):
```
1,aaa,bbb
2,ccc,ddd
3,eee,
4,fff,ggg
```
New results with column as the field delimiter rather than newline:
```
1
2
3
4
```
Next query, `drill-3149_4`:
```
select * from table(`table_function/lf_cr.tsv`(type=>'text', lineDelimiter=>'\n\r'))
```
Expected:
```
["1\taaa\tbbb"]
["2\tccc\tddd"]
["3\teee\t"]
["4\tfff\tggg"]
```
Actual:
```
["1","aaa","bbb"]
["2","ccc","ddd"]
["3","eee",""]
["4","fff","ggg"]
```
Notice how the expected results are wrong. We are using a tsv (tab separated file) but we expect the query to treat the tab as a normal character. With this PR, we change only the line delimiter, not the field delimiter, which seems more accurate.
Finally, `drill-3149_13`:
```
select * from table(`table_function/chinese.txt`(type=>'text',lineDelimiter=>'电脑坏了'))
```
Expected (all columns in a single field):
```
["1,aaa,bbb"]
["2,ccc,ddd"]
["3,eee,"]
["4,fff,ggg"]
```
With this PR we get the correct results:
```
["1","aaa","bbb"]
["2","ccc","ddd"]
["3","eee",""]
["4","fff","ggg"]
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #2054: DRILL-6168: Revise
format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615934230
@arina-ielchiieva, yes, we can modify the queries to force the incorrect behavior. We could add a yet another option to turn off inheritance and keep the old behavior. However, I'd argue that the old results were actually wrong, and the new results are correct. So, if we modify anything, it seems the best approach is to modify the expected results to reflect the correct behavior, and let the old versions fail because they were, in fact, wrong.
Thoughts?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #2054: DRILL-6168:
Revise format plugin table functions
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615128763
@paul-rogers changes look good to me. I have ran Functional / Advanced tests and there are four failures. Please take a look and make changes in code or suggest how tests should be updated.
```
Data Verification Failures:
/root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_2.q
/root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_1.q
/root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_4.q
/root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_13.q
```
Detailed output:
```
Data Verification Failures:
Query: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_2.q
select * from table(`table_function/cr_lf.csv`(type=>'text', lineDelimiter=>'\r\n'))
Baseline: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_2.e
Expected number of rows: 4
Actual number of rows from Drill: 4
Number of matching rows: 0
Number of rows missing: 4
Number of rows unexpected: 4
These rows are not expected (first 10):
["1","aaa","bbb"]
["2","ccc","ddd"]
["3","eee",""]
["4","fff","ggg"]
These rows are missing (first 10):
["3,eee,"] (1 occurence(s))
["1,aaa,bbb"] (1 occurence(s))
["2,ccc,ddd"] (1 occurence(s))
["4,fff,ggg"] (1 occurence(s))
Query: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_1.q
select columns[0] from table(`table_function/cr_lf.csv`(type=>'text', lineDelimiter=>'\r\n'))
Baseline: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_1.e
Expected number of rows: 4
Actual number of rows from Drill: 4
Number of matching rows: 0
Number of rows missing: 4
Number of rows unexpected: 4
These rows are not expected (first 10):
1
2
3
4
These rows are missing (first 10):
3,eee, (1 occurence(s))
1,aaa,bbb (1 occurence(s))
2,ccc,ddd (1 occurence(s))
4,fff,ggg (1 occurence(s))
Query: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_4.q
select * from table(`table_function/lf_cr.tsv`(type=>'text', lineDelimiter=>'\n\r'))
Baseline: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_4.e
Expected number of rows: 4
Actual number of rows from Drill: 4
Number of matching rows: 0
Number of rows missing: 4
Number of rows unexpected: 4
These rows are not expected (first 10):
["1","aaa","bbb"]
["2","ccc","ddd"]
["3","eee",""]
["4","fff","ggg"]
These rows are missing (first 10):
["3\teee\t"] (1 occurence(s))
["2\tccc\tddd"] (1 occurence(s))
["1\taaa\tbbb"] (1 occurence(s))
["4\tfff\tggg"] (1 occurence(s))
Query: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_13.q
select * from table(`table_function/chinese.txt`(type=>'text',lineDelimiter=>'电脑坏了'))
Baseline: /root/drillAutomation/drill-test-framework/framework/resources/Functional/table_function/positive/data/drill-3149_13.e
Expected number of rows: 4
Actual number of rows from Drill: 4
Number of matching rows: 0
Number of rows missing: 4
Number of rows unexpected: 4
These rows are not expected (first 10):
["1","aaa","bbb"]
["2","ccc","ddd"]
["3","eee",""]
["4","fff","ggg"]
These rows are missing (first 10):
["3,eee,"] (1 occurence(s))
["1,aaa,bbb"] (1 occurence(s))
["2,ccc,ddd"] (1 occurence(s))
["4,fff,ggg"] (1 occurence(s))
```
https://github.com/mapr/drill-test-framework/tree/master/framework/resources/Functional/table_function/positive/data
https://github.com/mapr/drill-test-framework/tree/master/framework/resources/Datasources/table_function
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #2054: DRILL-6168:
Revise format plugin table functions
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-616109007
@paul-rogers well, it's nice improvement for the test framework to have version indicated but this would require code changes in test framework itself, not sure if there is a capacity for this now.
Since you say that modifying the results is the correct approach. Could you please one PR in test framework repo with update of expected results? Once this is done, go ahead and merge this PR. We'll merge the PR with tests update afterwards.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #2054: DRILL-6168: Revise
format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-616224003
@arina-ielchiieva, thanks for the review and suggestions; I'll prepare a the PR you suggest.
Looking at the test framework code, I can almost offer you another option. The framework has a `before-run.sql` script which will issue `ALTER SESSION` commands. We can add an option to disable table function inheritance so the test run as-is.
Of course, we probably check if the option is valid. If I add a new option, it won't be valid for older Drill versions and so test startup would fail. So, we'd need something to trigger whether to set the option, or some way to catch and ignore errors for older versions. So, maybe this won't work as well as I'd hoped.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #2054: DRILL-6168: Revise
format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615472724
@arina-ielchiieva, the above analysis shows that this PR provides the correct results, the prior code and tests verified incorrect results. So, let's do this:
1. Approve and commit this PR.
2. Immediately update the four faulty expected results (`_e`) files in the test framework.
3. Rerun the selected tests to verify the test fixes.
Note that we cannot reverse the order: if we change the tests first, all runs except this PR will fail.
It would be ideal to have two sets of results: one before this PR, one after. Or, disable the four tests for commits before this PR. But, the test framework does not seem to have version awareness.
Unfortunately, I'm not yet set up to run the test framework. I can, however, fix the result files. Should I do a PR against the test framework?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #2054: DRILL-6168:
Revise format plugin table functions
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615869665
@paul-rogers is there a way to modify queries so they will pass before and after your PR? For example, add `fieldDelimiter` to table function, `order by` clause?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #2054: DRILL-6168: Revise
format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054#issuecomment-615408581
@arina-ielchiieva, thanks much for running the tests! Looks like these failures are due to the verifying the incorrect prior behavior where the default field delimiter was newline (same as the line delimiter), not comma. This PR changes the default to comma.
Let's consider an example. For this query:
```
select * from table(`table_function/cr_lf.csv`(type=>'text', lineDelimiter=>'\r\n'));
```
With this input:
```
1,aaa,bbb
2,ccc,ddd
3,eee,
4,fff,ggg
```
We currently expect this output because the old default field delimiter is a newline (same as the line delimiter):
```
["1,aaa,bbb"]
["2,ccc,ddd"]
["3,eee,"]
["4,fff,ggg"]
```
The correct expected results, with a default field delimiter of comma, is:
```
["1","aaa","bbb"]
["2","ccc","ddd"]
["3","eee",""]
["4","fff","ggg"]
```
Will investigate how to fix the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers merged pull request #2054: DRILL-6168: Revise
format plugin table functions
Posted by GitBox <gi...@apache.org>.
paul-rogers merged pull request #2054: DRILL-6168: Revise format plugin table functions
URL: https://github.com/apache/drill/pull/2054
----------------------------------------------------------------
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
With regards,
Apache Git Services