Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicit unicode column names are converted to lowercase when using postgresql #478

Closed
2 tasks done
majkinetor opened this issue Nov 28, 2022 · 9 comments
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@majkinetor
Copy link

majkinetor commented Nov 28, 2022

Bug Description

Serbian cyrilic: Опис is converted to опис.

select description as "Опис"
from forms f

image

Serbian latin:

select description as "Šuma"
from forms f

image

Severity

  • Low - I'm able to keep using Evidence as normal, but flagging this for the team to fix

Expected Behavior

Workarounds
There doesn't seem to be workaround

Environment Information
Operating System: Windows 10
Node version (node -v): 19.1.0
npm version (npm -v): 8.9.13
Package versions (npm list --depth=0): `@evidence-dev/evidence@5.0.12

Database:

  • Postgres
@majkinetor majkinetor added bug Something isn't working to-review Evidence team to review labels Nov 28, 2022
@archiewood
Copy link
Member

Hi @majkinetor,
Confirm this does not happen with other connectors (eg SQLite).

I think the specific piece of code here is this line

@hughess Is there a specific reason we lowercase the keys like this?

@archiewood archiewood removed the to-review Evidence team to review label Nov 28, 2022
@hughess
Copy link
Member

hughess commented Dec 12, 2022

@archiewood I actually used the same function for the postgres connector as we already had for the snowflake connector - I believe there was a reason to do that for snowflake but I'm not sure if postgres has the same requirement.

@mcrascal was there a reason for lowercasing the snowflake column names in the standardizeResult function?

If there's no specific reason for postgres, we should be good to remove that line

@hughess
Copy link
Member

hughess commented Dec 12, 2022

2 other ways we can solve this issue:

  1. Change the function that formats column titles to correctly handle cyrillic input (I think this might be difficult to accomplish)
  2. Allow an override for column titles in viz components - this exists in charts now (can override xAxisTitle and yAxisTitle), but doesn't exist yet for tables. It's something we're including in the new tables we're building, so that should be an option to use soon.
    • E.g., you could do something like <Column id=опис title="Опис"/>

@mcrascal
Copy link
Member

As far as I recall, the Snowflake API returns uppercased column names, regardless of how you write your query. It was quite annoying to need to uppercase everything when referencing columns in components. Just a preference thing -- either we get all uppercase, or all lowercase.

@hughess
Copy link
Member

hughess commented Dec 12, 2022

Ok perfect, thanks.

I've just confirmed the behaviour of the postgres db package:

  • Column names passed using double quotes are returned with the same cases as what's in the double quotes (e.g., "One" --> One)
  • All other column names are returned in lowercase (e.g., ONE --> one)

So I think that means we're good to remove the standardizeResult function for the postgres connector!

@hughess
Copy link
Member

hughess commented Dec 12, 2022

It looks like even with the removal of the lowercasing, we'll have an issue with the title formatting function - it's picking up whatever the first A-Z character is and it uppercasing that. We'll need to get it to stop at the first character in the string:

CleanShot 2022-12-12 at 12 38 35@2x

@hughess
Copy link
Member

hughess commented Dec 12, 2022

As it turns out, there's an issue in our function for formatting titles. The regex in that function was filtering out the non A-Z characters. With that fixed, we get the following:
CleanShot 2022-12-12 at 16 37 36@2x

@hughess
Copy link
Member

hughess commented Dec 14, 2022

@majkinetor I've added a fix for the title formatting issue in #522.

Does #522 solve your problem, or only part of it? If needed, we can fix the lowercase issue in postgres, but I'm hesitant to change that at the moment in case of unintended consequences.

@majkinetor
Copy link
Author

Good enough. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants