feat: Support nullable materialized columns using native types #26448

tkaemming · 2024-11-26T23:45:09Z

Problem

Properties that are materialized as columns with the current implementation cannot distinguish between null values and the string value "null" and require processing when reading to lossily convert these values to nulls:

posthog/posthog/hogql/printer.py

Lines 1350 to 1354 in cf39f4b

    
           # TODO: rematerialize all columns to properly support empty strings and "null" string values. 
        
           if self.context.modifiers.materializationMode == MaterializationMode.LEGACY_NULL_AS_STRING: 
        
               materialized_property_sql = f"nullIf({materialized_property_source}, '')" 
        
           else:  # MaterializationMode AUTO or LEGACY_NULL_AS_NULL 
        
               materialized_property_sql = f"nullIf(nullIf({materialized_property_source}, ''), 'null')"

Needing to transform these values when reading makes using data skipping indexes more difficult as the analyzer will not use the indexes for these expressions.

Additionally, property values are materialized as columns using the output of JSONExtractRaw with leading and trailing quotes stripped which can result in unnecessarily escaped values being contained within the stored.

Also see #19461 for details.

Changes

Adds the ability to materialize properties into Nullable(String) types. New columns added via the management command are created as nullable by default.

This uses the JSONExtract(column, key, 'Nullable(String)') form to avoid escaping errors (like as already happens with property groups):

SELECT JSONExtract('{"foo": "\\"hello\\""}', 'foo', 'Nullable(String)')

   ┌─JSONExtract('{"foo": "\\"hello\\""}', 'foo', 'Nullable(String)')─┐
1. │ "hello"                                                          │
   └──────────────────────────────────────────────────────────────────┘

Compare to:

SELECT replaceRegexpAll(JSONExtractRaw('{"foo": "\\"hello\\""}', 'foo'), '^"|"$', '')

   ┌─replaceRegexpAll(JSONExtractRaw('{"foo": "\\"hello\\""}', 'foo'), '^"|"$', '')─┐
1. │ \"hello\"                                                                      │
   └────────────────────────────────────────────────────────────────────────────────┘

Nullable columns are only used within HogQL queries as the change could affect existing legacy queries in undefined ways due to the format change as well as the introduction as null values.

Does this work well for both Cloud and self-hosted?

Yes.

How did you test this code?

Updated existing tests to account for nullable columns where relevant.

tkaemming · 2024-12-04T19:22:08Z

posthog/hogql/test/test_printer.py

+        materialize("events", "withmat_nullable", is_nullable=True)
+        self.assertEqual(
+            self._expr("properties.withmat_nullable.json.yet", context),
+            "replaceRegexpAll(nullIf(nullIf(JSONExtractRaw(events.mat_withmat_nullable, %(hogql_val_0)s, %(hogql_val_1)s), ''), 'null'), '^\"|\"$', '')",


Going to change this so it uses JSONExtract(events.mat_withmat_nullable, %(hogql_val_0)s, %(hogql_val_1)s, 'Nullable(String)') too (also will apply to the above), but will do that separately as it's not really coupled to this specific change and will cause a bunch of snapshot updates.

tkaemming · 2024-12-04T19:24:14Z

posthog/clickhouse/materialized_columns.py

+class MaterializedColumn(Protocol):
+    name: ColumnName
+    is_nullable: bool


This is necessary because the interface is defined in ee/* but used throughout the non-ee/ codebase and I don't want to wrap everything in typing.TYPE_CHECKING guards.

A lot of this would be simpler if we moved it to non-ee/ but not sure what the history is here so just working around it for now.

tkaemming · 2024-12-04T22:11:47Z

ee/clickhouse/materialized_columns/columns.py

@@ -40,15 +38,36 @@
 }


-class MaterializedColumn(NamedTuple):
+@dataclass
+class MaterializedColumn:
    name: ColumnName
    details: MaterializedColumnDetails


I'll probably consolidate these two classes (MaterializedColumn & MaterializedColumnDetails) at some point (there's not much benefit to them being separate at this point) but not in a big hurry to do that.

fuziontech · 2024-12-04T22:53:57Z

ee/clickhouse/materialized_columns/columns.py

-            ADD COLUMN IF NOT EXISTS {self.column.name} VARCHAR
-                MATERIALIZED {TRIM_AND_EXTRACT_PROPERTY.format(table_column=self.column.details.table_column)}
-            """,
+            f"ADD COLUMN IF NOT EXISTS {self.column.name} {self.column.type} MATERIALIZED {expression}",


:chef-kiss:

fuziontech · 2024-12-04T23:27:44Z

posthog/tasks/usage_report.py

+    lib_materialized_column = get_materialized_column_for_property("events", "properties", "$lib")
+    lib_expression = (
+        lib_materialized_column.name if lib_materialized_column is not None else "JSONExtractString(properties, '$lib')"
+    )


use that get_property_string_expr method here is the only feedback I have

FYI, the generated query is slightly different than the original, but not in a way that I think really matters all that much: 422d678

sentry-io · 2024-12-06T15:29:46Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ NetworkError: Connection refused (ch-offline.posthog.net:9440) /api/projects/{parent_lookup_team_id}/events/ View Issue
‼️ NetworkError: Connection refused (ch-online.posthog.net:9440) /api/environments/{parent_lookup_team_id}/query/ View Issue
‼️ NetworkError: Connection refused (ch-online.posthog.net:9440) /api/person/values/ View Issue
‼️ NetworkError: Connection refused (ch-online.posthog.net:9440) posthog.tasks.tasks.process_query_task View Issue
‼️ NetworkError: Connection refused (ch-online.posthog.net:9440) posthog.tasks.tasks.process_query_task View Issue

_{Did you find this useful? React with a 👍 or 👎}

tkaemming changed the title ~~feat: Support nullable materialized columns using native typesColumn types~~ feat: Support nullable materialized columns using native types Nov 26, 2024

tkaemming mentioned this pull request Nov 27, 2024

refactor: Update materialized column management #26414

Merged

tkaemming added 9 commits December 3, 2024 19:37

consolidate column type and expression logic

19bf4b7

reduce surface area of get_enabled_materialized_columns

24caa0c

get_materialized_columns should return columns

3108d15

move filtering up the stack

1a9202a

expose MaterializedColumn protocol for nullability's sake

a2314e8

update hogql printer to handle nullable columns

3c44b03

oops

aab924f

DAMN

6f611e4

add to management command

a8269bc

tkaemming force-pushed the column-types branch from 1c0d495 to a8269bc Compare December 4, 2024 03:38

tkaemming added 4 commits December 3, 2024 20:13

add test for printer behavior

c42bff5

reduce queries executed due to reordering instance setting check

5de94b8

update mocks

684c4a4

mypy baseline update

ab96f6c

tkaemming mentioned this pull request Dec 4, 2024

Query Performance: Problems and Potential Optimizations #26651

Open

type test

6924ecf

tkaemming commented Dec 4, 2024

View reviewed changes

tkaemming marked this pull request as ready for review December 4, 2024 22:46

tkaemming requested a review from a team as a code owner December 4, 2024 22:46

fuziontech approved these changes Dec 4, 2024

View reviewed changes

tkaemming added 2 commits December 4, 2024 15:41

use the thing that exists that does the thing

f35ad60

Merge branch 'master' into column-types

78d6041

tkaemming force-pushed the column-types branch from 422d678 to 78d6041 Compare December 5, 2024 00:43

Update query snapshots

4f59fd7

tkaemming merged commit cc990fd into master Dec 5, 2024
89 checks passed

tkaemming deleted the column-types branch December 5, 2024 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support nullable materialized columns using native types #26448

feat: Support nullable materialized columns using native types #26448

tkaemming commented Nov 26, 2024 •

edited

Loading

tkaemming Dec 4, 2024

tkaemming Dec 4, 2024

tkaemming Dec 4, 2024 •

edited

Loading

fuziontech Dec 4, 2024

fuziontech Dec 4, 2024

tkaemming Dec 5, 2024 •

edited

Loading

sentry-io bot commented Dec 6, 2024 •

edited

Loading

	# TODO: rematerialize all columns to properly support empty strings and "null" string values.
	if self.context.modifiers.materializationMode == MaterializationMode.LEGACY_NULL_AS_STRING:
	materialized_property_sql = f"nullIf({materialized_property_source}, '')"
	else: # MaterializationMode AUTO or LEGACY_NULL_AS_NULL
	materialized_property_sql = f"nullIf(nullIf({materialized_property_source}, ''), 'null')"

feat: Support nullable materialized columns using native types #26448

feat: Support nullable materialized columns using native types #26448

Conversation

tkaemming commented Nov 26, 2024 • edited Loading

Problem

Changes

Does this work well for both Cloud and self-hosted?

How did you test this code?

tkaemming Dec 4, 2024

Choose a reason for hiding this comment

tkaemming Dec 4, 2024

Choose a reason for hiding this comment

tkaemming Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

fuziontech Dec 4, 2024

Choose a reason for hiding this comment

fuziontech Dec 4, 2024

Choose a reason for hiding this comment

tkaemming Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

sentry-io bot commented Dec 6, 2024 • edited Loading

Suspect Issues

tkaemming commented Nov 26, 2024 •

edited

Loading

tkaemming Dec 4, 2024 •

edited

Loading

tkaemming Dec 5, 2024 •

edited

Loading

sentry-io bot commented Dec 6, 2024 •

edited

Loading