search

databrickslabs / remorph

Accelerates migrations to Databricks by automating code conversion and migration validation
Other
47 stars 31 forks source link

add support for SnowFlake SUBSTR #1226

Closed ericvergnaud closed 2 days ago

ericvergnaud commented 1 week ago

SnowFlake supports both SUBSTR and SUBSTRING, see https://docs.snowflake.com/fr/sql-reference/functions/substr. This PR fills the missing SUBSTR

github-actions[bot] commented 1 week ago

Coverage tests results

456 tests  ±0   419 ✅ ±0   4s ⏱️ ±0s   6 suites ±0    37 💤 ±0    6 files   ±0     0 ❌ ±0 

Results for commit cd45efb7. ± Comparison against base commit 9dcc986e.

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

ericvergnaud commented 6 days ago

It looks like SUBSTRING wasn't implemented either. Looking into SnowflakeFunctionBuilder we can see

    case "SUBSTR , SUBSTRING" => FunctionDefinition.standard(3)

Which appears to be a remnant of the time I scrapped all the function names from the documentation. This malformed definition hasn't been fixed since then, now could be the right time.

I was indeed a bit surprised by the "SUBSTR , SUBSTRING" format. Fixed.

ericvergnaud commented 6 days ago

If we are certain that SUBSTR and SUBSTRING will have the same signature in all dialects, we can get rid of that ill-formed definition in SnowflakeFunctionBuilder.

I would certainly not make that bet, but making it the default doesn't hurt since it can be overridden

jimidle commented 6 days ago

Tests are not passing - I presume you saw that?

ericvergnaud commented 6 days ago

Tests are not passing - I presume you saw that?

Yes, it's fixed now

ericvergnaud commented 2 days ago

Closed in favor of #1238