legend-engine icon indicating copy to clipboard operation
legend-engine copied to clipboard

integrate concat_ws for memsql and snowflake

Open theodosis1989 opened this issue 1 year ago • 3 comments

What type of PR is this?

  • Bug Fix

What does this PR do / why is it needed ?

The current translation of joinStrings from PURE is to the CONCAT method in SQL. In some of the SQL implementations, including MemSQL and Snowflake that are addressed in this MR, the CONCAT method concatenates the variables without a delimiter, and produces null when one the values is null.

Example1: ['A', 'B']->joinStrings('|') in PURE, produces SQL CONCAT('A', 'B', '|') which evaluates to 'AB|'. Example2: ['A', null]->joinStrings('|') in PURE, produces SQL CONCAT('A', null, '|') which evaluates to NULL.

Fix: This MR maps the joinStrings for MemSQL and Snowflake to CONCAT_WS instead. CONCAT_WS concatenates the variables using a delimiter Example1: ['A', 'B']->joinStrings('|') in PURE, produces SQL CONCAT_WS('|', 'A', 'B') which evaluates to 'A|B'. Example2: ['A', 'B']->joinStrings('|') in PURE, produces SQL CONCAT_WS('|', 'A', NULL) which evaluates to 'A'.

Which issue(s) this PR fixes:

Fixes #

Other notes for reviewers:

Does this PR introduce a user-facing change?

theodosis1989 avatar Feb 02 '24 12:02 theodosis1989

Test Results

     767 files  ±0       767 suites  ±0   1h 7m 8s :stopwatch: +53s 12 631 tests +3  12 398 :heavy_check_mark: +3  233 :zzz: ±0  0 :x: ±0  15 742 runs  +3  15 491 :heavy_check_mark: +3  251 :zzz: ±0  0 :x: ±0 

Results for commit 5a538d91. ± Comparison against base commit e55e1f10.

:recycle: This comment has been updated with latest results.

github-actions[bot] avatar Feb 02 '24 14:02 github-actions[bot]

This PR is stale because it has been open for 30 days with no activity. Please remove stale label or add any comment to keep this open. Otherwise this will be closed in 5 days.

finos-admin avatar Apr 03 '24 12:04 finos-admin

This PR is stale because it has been open for 30 days with no activity. Please remove stale label or add any comment to keep this open. Otherwise this will be closed in 5 days.

finos-admin avatar May 06 '24 12:05 finos-admin

This PR is stale because it has been open for 30 days with no activity. Please remove stale label or add any comment to keep this open. Otherwise this will be closed in 5 days.

finos-admin avatar Jun 28 '24 12:06 finos-admin

This PR was closed because it has been inactive for 35 days. Please re-open if this PR is still relevant.

finos-admin avatar Jul 03 '24 12:07 finos-admin