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

expression in table columns is converted to literal column twice #617

Closed
rhp4515 opened this issue Jun 14, 2016 · 13 comments · Fixed by #1103
Closed

expression in table columns is converted to literal column twice #617

rhp4515 opened this issue Jun 14, 2016 · 13 comments · Fixed by #1103
Labels
!deprecated-label:bug Deprecated label - Use #bug instead

Comments

@rhp4515
Copy link
Contributor

rhp4515 commented Jun 14, 2016

if ddtm_expr is an expression with special characters then timestamp_grain escapes the special characters already escaped

models.py 660

Steps to reproduce:

  1. Have a column which has unix timestamp
  2. Create a new dttm column 'date' with date_format(from_unixtime(ts), '%Y-%m-%d')
  3. Run any time-series based visualization query

Problem:

  1. % is escaped twice so %Y becomes %%Y and then becomes %%%%Y
@rhp4515
Copy link
Contributor Author

rhp4515 commented Jun 15, 2016

@mistercrunch

@mistercrunch mistercrunch added !deprecated-label:bug Deprecated label - Use #bug instead help wanted labels Jun 15, 2016
@ntantri
Copy link

ntantri commented Jul 5, 2016

I tried looking into this issue and found that there is this sqlalchemy method literal_column which uses a replace text method and is reported as a bug in sqlalchemy project: https://bitbucket.org/zzzeek/sqlalchemy/issues/3740/clean-up-doubling-of-percent-signs-base-on

Will try and see the update of whether it can be directly used with help of sqlalchemy's text method.

@ntantri
Copy link

ntantri commented Jul 12, 2016

After interacting with the Sqlalchemy issue support, #3737 - Bitbucket,

I have implemented and tested the solution to escape the double percentage getting displayed for literal_column. This solution is basically a decorated method that takes care of removing back the added percentages for type of DateTime in ColumnClause.

@yejianye
Copy link
Contributor

Met the same problem, hope the PR could be merged soon.

@yejianye
Copy link
Contributor

Also notice 'week' and 'month' time grain is broken for SQLite because of this issue as well. Both have percent sign in its sql expression.

            'sqlite': (
                Grain('Time Column', _('Time Column'), '{col}'),
                Grain('day', _('day'), 'DATE({col})'),
                Grain("week", _('week'),
                      "DATE({col}, -strftime('%w', {col}) || ' days')"),
                Grain("month", _('month'),
                      "DATE({col}, -strftime('%d', {col}) || ' days')"),
            ),

xrmx pushed a commit to xrmx/superset that referenced this issue Sep 13, 2016
If ddtm_expr is an expression with special characters then timestamp_grain escapes
the special characters already escaped.

Solution discussed with sqlalchemy upstream:
https://bitbucket.org/zzzeek/sqlalchemy/issues/3737/literal_column-given-a-specific-sql

Fix apache#617
mistercrunch pushed a commit that referenced this issue Sep 17, 2016
If ddtm_expr is an expression with special characters then timestamp_grain escapes
the special characters already escaped.

Solution discussed with sqlalchemy upstream:
https://bitbucket.org/zzzeek/sqlalchemy/issues/3737/literal_column-given-a-specific-sql

Fix #617
dennisobrien pushed a commit to dennisobrien/caravel that referenced this issue Sep 19, 2016
If ddtm_expr is an expression with special characters then timestamp_grain escapes
the special characters already escaped.

Solution discussed with sqlalchemy upstream:
https://bitbucket.org/zzzeek/sqlalchemy/issues/3737/literal_column-given-a-specific-sql

Fix apache#617
@rhunwicks
Copy link
Contributor

As @yejianye pointed out this issue also breaks 'week' and 'month' time grains on SQLite but the fix that was applied only applies to column expressions, so I think that time grains are still broken for SQLite.

@stonecharioteer
Copy link

@rhunwicks Time grains are still broken! Is there another bug report related to this?

@rhunwicks
Copy link
Contributor

@vinay87 I'm not aware of another bug report - I think this one should be reopened.

@mistercrunch
Copy link
Member

There's a bunch of tangled issues being discussed here, can someone open a new issue that explains exactly what is going on? I'm pretty sure the double % is not an issue anymore.

@rhunwicks
Copy link
Contributor

@mistercrunch when you run a query against a SQLite database that has a time grain of week or month, you pass a query_obj something like:

        {
            'groupby': [],
            'metrics': ['sum__value'],
            'granularity': 'received',
            'from_dttm': datetime.datetime(2001, 1, 1),
            'to_dttm': datetime.datetime(2001, 12, 31),
            'filter': [],
            'is_timeseries': True,
            'timeseries_limit': 50,
            'timeseries_limit_metric': None,
            'row_limit': 5000,
            'extras': {
                'time_grain_sqla': 'month',
            },
            'order_desc': True,
            }

The time grain code for SQLite uses strftime to truncate the date, but the %d is doubled up to %%d and consequently the strftime doesn't work, and so the date doesn't get truncated.

The SQL that gets generated by that query_obj is

SELECT
DATE(received, -strftime('%%d', received) || ' days', '+1 day') AS __timestamp,
SUM(value) AS sum__value 
FROM test_datasource 
WHERE received >= '2001-01-01 00:00:00.000000' AND received <= '2001-12-31 00:00:00.000000' GROUP BY DATE(received, -strftime('%%d', received) || ' days', '+1 day')
ORDER BY sum__value DESC

@betodealmeida
Copy link
Member

@mistercrunch can confirm this is still a problem, I hit it today. Will take a look.

@betodealmeida
Copy link
Member

@betodealmeida
Copy link
Member

I tested with 1.2.1 and can confirm it works, here's the generated query without the %%:

screen shot 2018-01-23 at 1 20 11 pm

zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this issue Nov 17, 2021
Bumps [lerna](https://github.com/lerna/lerna/tree/HEAD/core/lerna) from 3.22.0 to 3.22.1.
- [Release notes](https://github.com/lerna/lerna/releases)
- [Changelog](https://github.com/lerna/lerna/blob/master/core/lerna/CHANGELOG.md)
- [Commits](https://github.com/lerna/lerna/commits/v3.22.1/core/lerna)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this issue Nov 24, 2021
Bumps [lerna](https://github.com/lerna/lerna/tree/HEAD/core/lerna) from 3.22.0 to 3.22.1.
- [Release notes](https://github.com/lerna/lerna/releases)
- [Changelog](https://github.com/lerna/lerna/blob/master/core/lerna/CHANGELOG.md)
- [Commits](https://github.com/lerna/lerna/commits/v3.22.1/core/lerna)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this issue Nov 25, 2021
Bumps [lerna](https://github.com/lerna/lerna/tree/HEAD/core/lerna) from 3.22.0 to 3.22.1.
- [Release notes](https://github.com/lerna/lerna/releases)
- [Changelog](https://github.com/lerna/lerna/blob/master/core/lerna/CHANGELOG.md)
- [Commits](https://github.com/lerna/lerna/commits/v3.22.1/core/lerna)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this issue Nov 26, 2021
Bumps [lerna](https://github.com/lerna/lerna/tree/HEAD/core/lerna) from 3.22.0 to 3.22.1.
- [Release notes](https://github.com/lerna/lerna/releases)
- [Changelog](https://github.com/lerna/lerna/blob/master/core/lerna/CHANGELOG.md)
- [Commits](https://github.com/lerna/lerna/commits/v3.22.1/core/lerna)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
!deprecated-label:bug Deprecated label - Use #bug instead
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants