Conversation
|
Hey @lucasheriques! 👋 |
lucasheriques
changed the title
lucasheriques
force-pushed
the
feat/autogen-files-for-surveys-udf
branch
from
0c245fd to
6a178ad
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added new user-defined functions for survey response handling and upgraded funnel analysis functions from v4 to v5 in ClickHouse, with comprehensive test coverage for both old and new survey question formats.
- Added
get_survey_response_v5UDF in/posthog/user_scripts/v5/get_survey_response.pyto extract survey responses from event properties - Added architecture-specific binary selection in
/posthog/user_scripts/v5/aggregate_funnelfor aarch64 and x86_64 platforms - Added comprehensive test suite in
/posthog/user_scripts/v5/test_get_survey_response.pywith 8 test cases covering various survey response scenarios - Fixed duplicate
num_stepsargument inaggregate_funnel_trends_v4/v5functions - Updated all funnel-related function commands from v4 to v5 paths in XML configurations
7 file(s) reviewed, 15 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
style: Double JSON encoding of error message could make debugging more difficult. Consider using a cleaner error format.
There was a problem hiding this comment.
logic: No error handling if binaries are missing or not executable. Add checks before execution.
| aarch64) $DIR_NAME/aggregate_funnel_aarch64 "$@";; | |
| *) $DIR_NAME/aggregate_funnel_x86_64 "$@";; | |
| aarch64) | |
| if [ ! -x "$DIR_NAME/aggregate_funnel_aarch64" ]; then | |
| echo "Error: aggregate_funnel_aarch64 not found or not executable" | |
| exit 1 | |
| fi | |
| $DIR_NAME/aggregate_funnel_aarch64 "$@";; | |
| *) | |
| if [ ! -x "$DIR_NAME/aggregate_funnel_x86_64" ]; then | |
| echo "Error: aggregate_funnel_x86_64 not found or not executable" | |
| exit 1 | |
| fi | |
| $DIR_NAME/aggregate_funnel_x86_64 "$@";; |
There was a problem hiding this comment.
style: Using * as default case could silently run x86_64 binary on unsupported architectures. Consider explicit architecture matching.
There was a problem hiding this comment.
logic: parse_args function is referenced but not defined, and calculate_funnel_from_user_events is commented out without explanation
There was a problem hiding this comment.
style: consider adding structured error response format instead of concatenating error string with traceback
| <name>aggregate_funnel_array_trends_v4</name> | ||
|
|
There was a problem hiding this comment.
style: Extra blank line after function name breaks XML formatting consistency
| <name>aggregate_funnel_cohort_trends_v4</name> | ||
|
|
There was a problem hiding this comment.
style: Extra blank line after function name breaks XML formatting consistency
| <name>aggregate_funnel_array_trends_v5</name> | ||
|
|
There was a problem hiding this comment.
style: Extra blank line after function name breaks XML formatting consistency
| <name>aggregate_funnel_cohort_trends_v5</name> | ||
|
|
There was a problem hiding this comment.
style: Extra blank line after function name breaks XML formatting consistency
| <command>v4/aggregate_funnel_array_trends_test.py</command> | ||
| <lifetime>600</lifetime> | ||
| </function> | ||
| <function> |
There was a problem hiding this comment.
style: Missing newline before v5 function definitions. Add empty line for better readability.
| @@ -0,0 +1,121 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
we already have this file here https://github.com/PostHog/posthog/pull/30182/files#diff-fffa98dc304d8240297bc8824c2dd21c12605991a6bc155197a33340975c4c2d
under the PR #30182
so I wonder why do we have 2 times? do we need them twice? how do we make sure they are in sync?
|
close because of https://github.com/PostHog/posthog/pull/30288/files? |
4 participants
deploy before #30182