-
Notifications
You must be signed in to change notification settings - Fork 981
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DRILL-8450: Add Data Type Inference to XML Format Plugin #2819
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 One comment to fix.
This was simpler than I expected. You already had the typify() method which does the real work.
I learned how to add a new config property to this thing. Very useful.
All fields are read as strings. Nested fields are read as maps. Future functionality could include support for lists. | ||
The XML reader has an `allTextMode` which, when set to `true` reads all data fields as strings. | ||
When set to `false`, Drill will attempt to infer data types. | ||
Nested fields are read as maps. Future functionality could include support for lists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really part of this change set, but I don't know what you are suggesting by "future functionality could include support for lists." I'd like to understand that plan/idea just as part of grokking all of this XML mapping.
Entry<Class, String> result = Typifier.typify(data); | ||
String dataType = result.getKey().getSimpleName(); | ||
|
||
// If the string is empty, return UNKNOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next line of code contradicts this comment by returning VARCHAR.
(Unless VARCHAR == UNKNOWN, which is news to me.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbeckerle Drill doesn't really have an UNKNOWN
data type. The way the typifier works is that if it can't determine the datatype, it falls back to string which can basically accept anything.
Regarding the lists... The issue is that to create a list, you have to set the data mode to REPEATED
. The problem with XML is that there's no real way to know if a field is repeated or not. Consider this:
<book>
<author>a</author>
</book>
<book>
<author>a1</author>
<author>a2</author>
</book>
Since Drill uses the streaming reader, when it first encounters the author
field, it would add an entry for a VARCHAR field. However, when it gets to the next author record, it should be list, but there's no way to really know that w/o a schema.
With JSON we don't have this problem because it uses [
to denote lists.
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense.
For XML you need XSD to know what's potentially repeating.
Sometimes that is easy because of minOccurs/maxOccurs.
But there's also these "implied arrays".
<element name="a" type="xs:int"/><!-- this is a[1] -->
<element name="b" type="xs:int"/>
<element name="a" type="xs:int"/><!-- this is a[2] -->
That's allowed in both XSD and DFDL schemas (though I want to change Daffodil to issue a warning if you do this, because it is such a bad idea when representing structured data.)
The element 'a' looks like an array, in that you can index it.
I think for drill there are just 2 columns: 'a', 'b', but as there is more than one declaration for 'a', it is an implied array.
Even just detecting this (and disallowing it for now) requires a more sophisticated metadata builder which is what I'm working on now.
Converting to draft. There's a unit test failing in the HTTP plugin. |
@mbeckerle Unit tests fixed. I also added the data type inference for APIs that generate XML. |
@mbeckerle Could you please take another look. I had to fix a few things for a unit test. Thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 last UT fix change looks fine, just a question.
@@ -111,7 +111,7 @@ public String toString() { | |||
public static class HttpXmlOptionsBuilder { | |||
|
|||
private int dataLevel; | |||
private boolean allTextMode; | |||
private Boolean allTextMode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought there were 3 modes: allTextMode, allNumbersAreDouble mode, and infer-types mode.
So why is this a boolean vs am enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbeckerle
In the JSON reader there are two parameters: allTextMode
and readAllNumbersAsDouble
. Both are boolean. For the XML reader, I chose not to implement the readAllNumbersAsDouble
parameter because in practice, it requires very clean data. From using Drill with clients, I can tell you from a lot of personal experience that this was one of the biggest data challenges. For instance, you'd get data where there was an DOUBLE field and then there would be a row with zero denoted as 0
. This would then cause schema change exceptions.
We have actually made significant improvements in Drill's implicit casting rules which do prevent a lot of schema change exceptions and as a result, IMHO, it makes distinguishing between INTs and DOUBLES a lot less important. So.. out of laziness I decided it wasn't worth it. I can be convinced otherwise.
@mbeckerle @jnturton Are we ok to merge this? I'll add support for arrays in a separate PR. |
LGTM |
DRILL-8450: Add Data Type Inference to XML Format Plugin
Description
This PR adds data type inference to the XML format plugin. In similar fashion to other plugins, it adds a new configuration parameter:
allTextMode
, which when set totrue
, reads all data as strings. The default istrue
.Note that the inference is limited to doubles, date, timestamps, boolean and strings.
Documentation
Updated README
Testing
Added unit test.