SQL: Add multi_value_field_leniency inside FieldHitExtractor #40113
Conversation
For cases where fields can have multi values, allow the behavior to be customized through a dedicated configuration field. By default this will be enabled on the drivers so that existing datasets work instead of throwing an exception. For regular SQL usage, the behavior is false so that the user is aware of the underlying data. Fix elastic#39700
|
Pinging @elastic/es-search |
| super(query, params, filter, zoneId, fetchSize, requestTimeout, pageTimeout, requestInfo); | ||
| this.cursor = cursor; | ||
| this.columnar = columnar; | ||
| this.fieldmultiValueLeniency = multiValueFieldLeniency; |
There was a problem hiding this comment.
Why the different naming style between the constructor parameter and the inner variable? fieldmultiValueLeniency VS. multiValueFieldLeniency. Also, if you want to stick with this difference, it should be fieldMultiValueLeniency (capital letter M).
Fix field pattern
|
|
||
| [float] | ||
| ==== Mapping | ||
| `field.multi.value.leniency` (default `true`):: Whether to lenient and return the first value for fields with multiple values (true) or throw an exception. |
astefan
left a comment
astefan
left a comment
There was a problem hiding this comment.
LGTM. Left few minor comments.
Also, should we add an update to the Limitations page? https://www.elastic.co/guide/en/elasticsearch/reference/current/sql-limitations.html#_array_type_of_fields
|
|
||
| [float] | ||
| ==== Mapping | ||
| `field.multi.value.leniency` (default `true`):: Whether to lenient and return the first value for fields with multiple values (true) or throw an exception. |
There was a problem hiding this comment.
"Whether to be lenient and return....."
| } | ||
|
|
||
| private void createTestDataForByteValueTests(byte random1, byte random2, byte random3) throws Exception, IOException { | ||
| private void createRandomMultiValue() throws Exception { |
There was a problem hiding this comment.
I would use a different name for this method, more in-line with the rest of the methods: createTestDataForMultiValueTests.
| Boolean.FALSE, | ||
| null, | ||
| new RequestInfo(Mode.CLI), | ||
| false); |
There was a problem hiding this comment.
Instead of false value for multi_value_leniency here why not using Protocol.FIELD_MULTI_VALUE_LENIENCY ?
There was a problem hiding this comment.
Also, this means that on the CLI we won't allow users to use the multi-value leniency option... I think it makes sense.
There was a problem hiding this comment.
Indeed we won't since that's a human consumer that needs to be made aware of it.
| null, | ||
| null, | ||
| requestInfo(), | ||
| false).toXContent(builder, params); |
There was a problem hiding this comment.
Instead of false value for multi_value_leniency here why not using Protocol.FIELD_MULTI_VALUE_LENIENCY ?
| // trigger version initialization | ||
| // typically this should have already happened but in case the | ||
| // JdbcDriver/JdbcDataSource are not used and the impl. classes used directly | ||
| // Esriver/EsDataSource are not used and the impl. classes used directly |
It's already in there: https://github.com/elastic/elasticsearch/pull/40113/files#diff-0c428a8392825e99371be55d9ba98b94 |
|
Yep, sorry about that. For whatever unknown reason, I didn't fully connect the dots. |
For cases where fields can have multi values, allow the behavior to be customized through a dedicated configuration field. By default this will be enabled on the drivers so that existing datasets work instead of throwing an exception. For regular SQL usage, the behavior is false so that the user is aware of the underlying data. Fix #39700 (cherry picked from commit 2b35157)
For cases where fields can have multi values, allow the behavior to be customized through a dedicated configuration field. By default this will be enabled on the drivers so that existing datasets work instead of throwing an exception. For regular SQL usage, the behavior is false so that the user is aware of the underlying data. Fix #39700 (cherry picked from commit 2b35157)
For cases where fields can have multi values, allow the behavior to be customized through a dedicated configuration field. By default this will be enabled on the drivers so that existing datasets work instead of throwing an exception. For regular SQL usage, the behavior is false so that the user is aware of the underlying data. Fix #39700 (cherry picked from commit 2b35157)
6 participants
For cases where fields can have multi values, allow the behavior to be
customized through a dedicated configuration field.
By default this will be enabled on the drivers so that existing datasets
work instead of throwing an exception.
For regular SQL usage, the behavior is false so that the user is aware
of the underlying data.
Fix #39700