fix(impala/pyspark): use regexp_replace to strip

[deepyaman]

Description of changes

Use regexp_replace instead of trim methods to implement strip methods, due to issues handling the form feed character.

This is almost certainly slower. I alternatively considered dropping \f from the set of characters to trim; however, data can include form feeds (e.g. from legacy systems), so correctness seems preferable.

As an added bonus, this fixes two xfails on the PySpark side.

Issues closed

• Resolves bug: String expression strip removes leading and trailing "f" characters #11894

[Copilot]

Pull request overview

Updates Impala and PySpark string lstrip/rstrip/strip compilation to use regexp_replace for correct whitespace handling (notably form feed), and adjusts tests/snapshots accordingly.

Changes:

• Implement LStrip/RStrip/Strip in the PySpark and Impala SQL compilers via regexp_replace patterns using \\s.
• Extend backend string method tests with a form-feed case and remove PySpark/Databricks xfails for lstrip/rstrip.
• Refresh Impala SQL snapshots to match the new REGEXP_REPLACE output; includes a few small Polars backend cleanups.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ibis/backends/tests/test_string.py	Adds a \f test value and updates expectations; removes PySpark/Databricks lstrip/rstrip xfails.
ibis/backends/sql/compilers/pyspark.py	Implements visit_LStrip/visit_RStrip/visit_Strip using regexp_replace.
ibis/backends/sql/compilers/impala.py	Switches strip variants to regexp_replace to avoid incorrect trimming behavior.
ibis/backends/polars/init.py	Minor cleanup (typo fix, exception type, type annotation tweaks).
ibis/backends/impala/tests/snapshots/test_string_builtins/test_string_builtins/lstrip/out.sql	Updates snapshot to REGEXP_REPLACE(..., '^\\s+', '').
ibis/backends/impala/tests/snapshots/test_string_builtins/test_string_builtins/rstrip/out.sql	Updates snapshot to REGEXP_REPLACE(..., '\\s+$', '').
ibis/backends/impala/tests/snapshots/test_string_builtins/test_string_builtins/strip/out.sql	Updates snapshot to `REGEXP_REPLACE(..., '^\s+

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[NickCrews]

I agree that the expected behavior is that \f should be treated as whitespace, and therefore stripped.

To be clear, my understanding is (please correct if I'm wrong):

On main, the current behavior is:

• on pyspark and databricks, it didn't even compile, because Spark SQL LTRIM doesn't accept characters to trim. It does look like this param was added in pyspark 4.0. But we support pyspark back to 3.5. Can you please add a TODO comment to the implementation that says that when we drop support for pyspark<4, we should update to use this new syntax?
• on impala, most other .strip() calls worked, but it misses the \f case. It uses LTRIM(t0.string_col, ' \t\n\r\v\f') but there is some bug in upstream impala such that it doesn't interpret the \f? Do you also see this as an upstream bug? Have you filed a bug with the impala folks?
• All other backends work as expected.

After this change:

• pyspark works as expected. We have to use regex because we need to keep supporting pyspark>3.5.
• impala now works because we have to workaround the impala bug/limitation by using a regex.

[deepyaman]

@NickCrews Thanks for the quick review, and sorry for the delay in responding! I was running some tests with Spark 3.5 and Spark 4.1 to answer your questions last night, but I ended up coming to the conclusion that this solution isn't correct.

I agree that the expected behavior is that \f should be treated as whitespace, and therefore stripped.

To be clear, my understanding is (please correct if I'm wrong):

On main, the current behavior is:

• on pyspark and databricks, it didn't even compile, because Spark SQL LTRIM doesn't accept characters to trim. It does look like this param was added in pyspark 4.0. But we support pyspark back to 3.5. Can you please add a TODO comment to the implementation that says that when we drop support for pyspark<4, we should update to use this new syntax?

Actually, Spark SQL LTRIM (and RTRIM) does accept characters to trim, even on 3.5.

The problem is the use of the \f literal, which Spark interprets as the character 'f'. This can be circumvented by doing something like ' \t\n\r\v' || chr(12) (I need to double-check that \v works, too).

I'll confirm this across 3.5 (via local Spark Connect) and 4.1 (on Databricks).

• on impala, most other .strip() calls worked, but it misses the \f case. It uses LTRIM(t0.string_col, ' \t\n\r\v\f') but there is some bug in upstream impala such that it doesn't interpret the \f? Do you also see this as an upstream bug? Have you filed a bug with the impala folks?

(Haven't revisited Impala yet.)

• All other backends work as expected.

After this change:

• pyspark works as expected. We have to use regex because we need to keep supporting pyspark>3.5.
• impala now works because we have to workaround the impala bug/limitation by using a regex.

Hoping to get this revised over the weekend!