Skip to content

Improve Unicode support for MSSQL#6690

Merged
mistercrunch merged 2 commits intoapache:masterfrom
villebro:unicode
Jan 29, 2019
Merged

Improve Unicode support for MSSQL#6690
mistercrunch merged 2 commits intoapache:masterfrom
villebro:unicode

Conversation

@villebro
Copy link
Member

@villebro villebro commented Jan 15, 2019

The upcoming SQLAlchemy 1.3 release introduces N-prefixed Unicode string handling on MSSQL (see sqlalchemy/sqlalchemy@e1b299d ). This PR introduces support for that feature and has been tested on master branch of SQLAlchemy. This is backwards compatible with earlier versions of SQLAlchemy, so doesn't explicitly require 1.3, it just won't handle unicode values correctly on <1.3. Does not impact other db engines, aka minimal risk of regressions. Fixes #6624

Before:

screenshot 2019-01-16 at 17 43 35

After:

screenshot 2019-01-16 at 17 46 58

@villebro villebro changed the title [WIP] Improve Unicode support for MSSQL Improve Unicode support for MSSQL Jan 16, 2019
@villebro
Copy link
Member Author

Ready for review @mistercrunch

@codecov-io
Copy link

codecov-io commented Jan 16, 2019

Codecov Report

Merging #6690 into master will decrease coverage by 17.34%.
The diff coverage is 70%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #6690       +/-   ##
===========================================
- Coverage   73.42%   56.07%   -17.35%     
===========================================
  Files          75      525      +450     
  Lines       10084    23229    +13145     
  Branches        0     2777     +2777     
===========================================
+ Hits         7404    13026     +5622     
- Misses       2680     9794     +7114     
- Partials        0      409      +409
Impacted Files Coverage Δ
superset/connectors/sqla/models.py 78.17% <100%> (+0.08%) ⬆️
superset/db_engine_specs.py 54.11% <57.14%> (+0.02%) ⬆️
superset/assets/src/components/Checkbox.jsx 100% <0%> (ø)
...ations/deckgl/layers/Polygon/PolygonChartPlugin.js 0% <0%> (ø)
...ets/src/dashboard/components/dnd/DragDroppable.jsx 97.14% <0%> (ø)
...c/visualizations/deckgl/layers/Polygon/Polygon.jsx 0% <0%> (ø)
...ssets/src/visualizations/presets/MapChartPreset.js 0% <0%> (ø)
superset/assets/src/components/EditableTitle.jsx 77.14% <0%> (ø)
...assets/src/visualizations/Iframe/transformProps.js 0% <0%> (ø)
superset/assets/src/setup/setupPlugins.js 0% <0%> (ø)
... and 442 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83ee917...a08eb05. Read the comment docs.

@kristw kristw added enhancement:request Enhancement request submitted by anyone from the community sqllab Namespace | Anything related to the SQL Lab change:backend Requires changing the backend review labels Jan 17, 2019
@villebro villebro changed the title Improve Unicode support for MSSQL [WIP] Improve Unicode support for MSSQL Jan 25, 2019
@villebro
Copy link
Member Author

Bad rebase, back to WIP

@villebro villebro changed the title [WIP] Improve Unicode support for MSSQL Improve Unicode support for MSSQL Jan 25, 2019
@villebro
Copy link
Member Author

villebro commented Jan 25, 2019

This should be ok now, unsure why CI failed the cypress tests. @kristw: The .SQLLab tag should be removed, as this isn't related to SQL Lab, only query generation on MS SQL.

@kristw kristw removed the sqllab Namespace | Anything related to the SQL Lab label Jan 25, 2019
@mistercrunch mistercrunch merged commit c44ae61 into apache:master Jan 29, 2019
john-bodley added a commit that referenced this pull request Jan 30, 2019
john-bodley added a commit that referenced this pull request Jan 31, 2019
* Revert "creating new circular-json safe stringify and replacing one call (#6772)"

This reverts commit 11a7ad0.

* Revert "Improve Unicode support for MSSQL (#6690)"

This reverts commit c44ae61.

* Revert "Fix uniqueness constraints on tables table (#6718)"

This reverts commit c4fb7a0.
@villebro villebro deleted the unicode branch February 24, 2019 19:17
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 First shipped in 0.34.0 labels Feb 28, 2024
@xxcmp1977
Copy link

I just found that it's ignoring unicode string prefix N when using where_in clause in Jinja template
so that
and path in {{filter_values('Category')|where_in}}
expanded to
and path in ('á é í ó ú ü ñ')
instead of
and path in (N'á é í ó ú ü ñ')

Workaround is to manually rewrite it to something like

{% set values = filter_values('Category') %}
and path in (
    {% for value in values %}
        {{ "N'" + value + "'" }}{% if not loop.last %}, {% endif %}
    {% endfor %}
)

I'm using mssql+pymssql connection
Superset version 2 (not sure whuch one exactly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels change:backend Requires changing the backend enhancement:request Enhancement request submitted by anyone from the community 🚢 0.34.0 First shipped in 0.34.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

filtering unicode strings using where clause on MSSQL

5 participants