Interval type support #80
Conversation
This adds support for the interval types: - columns of type interval are now accepted; - the driver will perform the translation between ES/SQL's ISO 8601 representation and the SQL standard; - the results can be converted to all permited data types.
This adds support for parameters of type interval. The driver will perform the conversion between C values to interval SQL values within the allowed conversion matrix.
|
The PR is ready for reviewing. |
| #define TYPE_DOUBLE "DOUBLE" | ||
| #define TYPE_BINARY "BINARY" | ||
| #define TYPE_OBJECT "OBJECT" | ||
| #define TYPE_NESTED "NESTED" |
There was a problem hiding this comment.
Any particular reason for making these upper case?
Maybe it makes no difference, but in the ES docs they're all lower case: https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-types.html
There probably is a good reason, but it would be good to add a comment to say why for the benefit of future maintainers.
There was a problem hiding this comment.
Any particular reason for making these upper case?
I wanted to have them aligned with how ES/SQL reports them, which is upper case. No other reason than that.
The type matching does a case insensitive comparison so either would be fine.
driver/convert.c
Outdated
| } | ||
|
|
||
| /* split by last letter */ | ||
| switch (wstr->str[wstr->cnt - 1] | 0x20) { |
There was a problem hiding this comment.
I assume the | 0x20 bits are to make the letters lower case? Is this a standard C idiom? I haven't seen it used widely, although I'm a C++ programmer so that might just be ignorance of idiomatic C.
If it's not a mega popular idiom then:
- Are you sure it won't cause a subtle bug where a character that's not a letter gets converted to some other character that matches a
case? - Even if 1 is not a problem, it might be worth adding a comment to say why the
| 0x20is there.
Alternatively, could towupper be used?
There was a problem hiding this comment.
I assume the | 0x20 bits are to make the letters lower case?
Correct.
Is this a standard C idiom?
For the ASCII charset, yes, that's common-ish (as a cheap tolower() alternative). For a whole UTF8 set, this wouldn't work, but SQL keywords only use the ASCII set, so this should be safe.
a character that's not a letter gets converted to some other character that matches a case
This should not happen, since | 0x20 can only increase character's value and the comparison is done against a "full" wide character with a value from within the ASCII subset (i.e. no truncation occurs, which could lead to a false match).
it might be worth adding a comment to say why the | 0x20 is there.
Sure, I'll add it.
- the used `| 0x20` lowers the character, if that has a value in the ASCII set. add comments to make that clearer.
* add interval support This adds support for the interval types: - columns of type interval are now accepted; - the driver will perform the translation between ES/SQL's ISO 8601 representation and the SQL standard; - the results can be converted to all permited data types. * add interval parameter support This adds support for parameters of type interval. The driver will perform the conversion between C values to interval SQL values within the allowed conversion matrix. * add comment on char lowering in switches - the used `| 0x20` lowers the character, if that has a value in the ASCII set. add comments to make that clearer. (cherry picked from commit ca990c9)
bpintea
added
>feature
This adds support for the interval types:
representation and the SQL standard;