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

Use column types provided by server rather than trying to derive them from the values #4352

Closed

Conversation

Jakdaw
Copy link
Contributor

@Jakdaw Jakdaw commented Nov 13, 2019

What type of PR is this? (check all applicable)

  • Bug Fix

Description

For background see:

https://discuss.redash.io/t/hex-for-binary-data/4900/11

Be warned, first time I've touched Javascript!

Start using the column types in the JSON document rather than trying to derive them from the values. Now a MySQL query like SELECT "2019-01-01", date("2019-01-01"); will return a string and a date.

I don't think the 'float' thing was ever actually being set in the old logic; and I can't see what our require the JSON.stringify() logic.

@Jakdaw
Copy link
Contributor Author

Jakdaw commented Nov 13, 2019

Ahem! and I've only actually tested this on the Redash8 branch... tho I don't think that's relevant in practice!

@gabrieldutra
Copy link
Member

Hi @Jakdaw, saw your comment about the tests in the forum 🙂.

Changing this line to use the SELECT DATE('{{test-parameter}}') AS parameter query should fix the failing tests:

query: "SELECT '{{test-parameter}}' AS parameter",

There's also a change in a Percy snapshot that this should fix too:

using TIMESTAMP '1995-09-03T07:45' AS "date", instead.

LMK if you get any issues with it

@Jakdaw
Copy link
Contributor Author

Jakdaw commented Nov 14, 2019

Thanks @gabrieldutra looks like that sorted it!

@guidopetri
Copy link
Contributor

@Jakdaw , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants