gianm commented on code in PR #14792:
URL: https://github.com/apache/druid/pull/14792#discussion_r1297754423
##########
docs/ingestion/schema-design.md:
##########
@@ -261,7 +261,7 @@ native boolean types, Druid ingests these values as strings
if `druid.expression
the [array functions](../querying/sql-array-functions.md) or
[UNNEST](../querying/sql-functions.md#unnest). Nested
columns can be queried with the [JSON
functions](../querying/sql-json-functions.md).
-We also highly recommend setting `druid.generic.useDefaultValueForNull=false`
when using these columns since it also enables out of the box `ARRAY` type
filtering. If not set to `false`, setting `sqlUseBoundsAndSelectors` to `false`
on the [SQL query context](../querying/sql-query-context.md) can enable `ARRAY`
filtering instead.
+We also highly recommend setting `druid.generic.useDefaultValueForNull=false`
(the default) when using these columns since it also enables out of the box
`ARRAY` type filtering. If not set to `false`, setting
`sqlUseBoundsAndSelectors` to `false` on the [SQL query
context](../querying/sql-query-context.md) can enable `ARRAY` filtering instead.
Review Comment:
Hmm. This is ripe for confusion, since it's the only place
`sqlUseBoundsAndSelectors` is mentioned, and it doesn't say what the setting
does. It also makes it sound like there is no real downside to setting it to
`false` always. But there is a downside, right?
I am not sure mentioning the setting is worth the added confusion. How about
we don't mention it at all, and have the official position in the docs be that
you need SQL-compatible null handling in order to get `ARRAY` filtering?
Btw, I recognize this patch isn't really changing this section meaningfully.
But… still.
##########
docs/querying/sql-data-types.md:
##########
@@ -71,8 +71,8 @@ Casts between two SQL types with the same Druid runtime type
have no effect othe
Casts between two SQL types that have different Druid runtime types generate a
runtime cast in Druid.
If a value cannot be cast to the target type, as in `CAST('foo' AS BIGINT)`,
Druid either substitutes a default
-value (when `druid.generic.useDefaultValueForNull = true`, the default mode),
or substitutes [NULL](#null-values) (when
Review Comment:
Swap these so the default (SQL-compatible) behavior is first, and explicitly
mention that `true` is a legacy mode.
##########
docs/querying/sql-data-types.md:
##########
@@ -129,15 +129,15 @@ VARCHAR. ARRAY typed results will be serialized into
stringified JSON arrays if
## NULL values
The
[`druid.generic.useDefaultValueForNull`](../configuration/index.md#sql-compatible-null-handling)
-runtime property controls Druid's NULL handling mode. For the most SQL
compliant behavior, set this to `false`.
+runtime property controls Druid's NULL handling mode. For the most SQL
compliant behavior, set this to `false` (the default).
-When `druid.generic.useDefaultValueForNull = true` (the default mode), Druid
treats NULLs and empty strings
+When `druid.generic.useDefaultValueForNull = true`, Druid treats NULLs and
empty strings
Review Comment:
Explicitly call this a legacy mode.
##########
docs/design/segments.md:
##########
@@ -82,10 +82,13 @@ For each row in the list of column data, there is only a
single bitmap that has
## Handling null values
-By default, Druid string dimension columns use the values `''` and `null`
interchangeably. Numeric and metric columns cannot represent `null` but use
nulls to mean `0`. However, Druid provides a SQL compatible null handling mode,
which you can enable at the system level through
`druid.generic.useDefaultValueForNull`. This setting, when set to `false`,
allows Druid to create segments _at ingestion time_ in which the following
occurs:
+By default, Druid runs in a SQL compatible null handling mode, which allows
Druid to create segments _at ingestion time_ in which the following occurs:
Review Comment:
This section could use some rewording to make it sound more like the
SQL-compatible mode is the normal one. Various phrasings throughout the section
are written as if legacy mode is normal.
For example, the data structures in SQL-compatible mode are described as
"additional" over legacy. It'd be better to describe legacy as _missing_
certain structures rather than SQL-compatible as _adding_ them.
This is a nit, and it doesn't need to block this PR, but I think it would be
good to do it the rewords in a follow-on PR.
##########
docs/ingestion/schema-design.md:
##########
@@ -261,7 +261,7 @@ native boolean types, Druid ingests these values as strings
if `druid.expression
the [array functions](../querying/sql-array-functions.md) or
[UNNEST](../querying/sql-functions.md#unnest). Nested
columns can be queried with the [JSON
functions](../querying/sql-json-functions.md).
-We also highly recommend setting `druid.generic.useDefaultValueForNull=false`
when using these columns since it also enables out of the box `ARRAY` type
filtering. If not set to `false`, setting `sqlUseBoundsAndSelectors` to `false`
on the [SQL query context](../querying/sql-query-context.md) can enable `ARRAY`
filtering instead.
+We also highly recommend setting `druid.generic.useDefaultValueForNull=false`
(the default) when using these columns since it also enables out of the box
`ARRAY` type filtering. If not set to `false`, setting
`sqlUseBoundsAndSelectors` to `false` on the [SQL query
context](../querying/sql-query-context.md) can enable `ARRAY` filtering instead.
Review Comment:
Hmm. This is ripe for confusion, since it's the only place
`sqlUseBoundsAndSelectors` is mentioned, and it doesn't say what the setting
does. It also makes it sound like there is no real downside to setting it to
`false` always. But there is a downside, right?
I am not sure mentioning the setting is worth the added confusion. How about
we don't mention it at all, and have the official position in the docs be that
you need SQL-compatible null handling in order to get `ARRAY` filtering?
Btw, I recognize this patch isn't really changing this section meaningfully.
But… still.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
