[bugfix] temporal columns with expression fail #4890
Conversation
error msg: "local variable 'literal' referenced before assignment" Error occurs [only] when using temporal column defined as a SQL expression. Also noticed that examples were using `granularity` instead of using `granularity_sqla` as they should. Fixed that here.
mistercrunch
force-pushed
the
fix_literals
branch
from
6d89f0a
to
6a7b25f
Compare
| def test_get_timestamp_expression(self): | ||
| tbl = self.get_table_by_name('birth_names') | ||
| ds_col = tbl.get_column('ds') | ||
| sqla_literal = ds_col.get_timestamp_expression(None) |
There was a problem hiding this comment.
Can we add a test thats passing a value into get_timestamp_expression() to make sure we are returning the proper literal_col
There was a problem hiding this comment.
The problem is the test will fail depending on the database it's working on. I guess I can figure out which engine it's on (postgres/mysql) and do the proper assertion depending on that
There was a problem hiding this comment.
Or just add one test per database:
https://docs.python.org/2/library/unittest.html#skipping-tests-and-expected-failures
superset/connectors/sqla/models.py
Outdated
| literal = expr.format(col=expr) | ||
| if grain: | ||
| literal = grain.function | ||
| literal = expr.format(col=expr) |
There was a problem hiding this comment.
Isn't this line always overriding the one before? the one before should be expr = grain.function or there should be an else branch somewhere
There was a problem hiding this comment.
Yeah, I think this should be
literal = literal.format(col=expr)There was a problem hiding this comment.
Yeah I think we're both wrong on this one :( , I'm going to clean this up a bit more and write many tests covering the whole function.
Codecov Report
@@ Coverage Diff @@
## master #4890 +/- ##
==========================================
+ Coverage 76.97% 77.13% +0.16%
==========================================
Files 44 44
Lines 8537 8542 +5
==========================================
+ Hits 6571 6589 +18
+ Misses 1966 1953 -13
Continue to review full report at Codecov.
|
|
superseeds #4892 |
mistercrunch
force-pushed
the
fix_literals
branch
from
1cb6a02
to
3ac0ba5
Compare
* [bugfix] temporal columns with expression fail error msg: "local variable 'literal' referenced before assignment" Error occurs [only] when using temporal column defined as a SQL expression. Also noticed that examples were using `granularity` instead of using `granularity_sqla` as they should. Fixed that here. * Add tests
* [bugfix] temporal columns with expression fail error msg: "local variable 'literal' referenced before assignment" Error occurs [only] when using temporal column defined as a SQL expression. Also noticed that examples were using `granularity` instead of using `granularity_sqla` as they should. Fixed that here. * Add tests
* [bugfix] temporal columns with expression fail error msg: "local variable 'literal' referenced before assignment" Error occurs [only] when using temporal column defined as a SQL expression. Also noticed that examples were using `granularity` instead of using `granularity_sqla` as they should. Fixed that here. * Add tests
mistercrunch
added
🏷️ bot
error msg: "local variable 'literal' referenced before assignment"
Error occurs [only] when using temporal column defined as a SQL
expression.
Also noticed that examples were using
granularityinstead of usinggranularity_sqlaas they should. Fixed that here.