You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/10/04 02:43:17 UTC

[GitHub] [superset] john-bodley opened a new pull request #16950: Revert "fix: the calculated columns explicit type convert into date"

john-bodley opened a new pull request #16950:
URL: https://github.com/apache/superset/pull/16950


   This PR reverts apache/superset#14813 as there should be no need for type inferencing given that the type of the calculated column should be set in the same modal—exposed if one scrolls. See attached screenshot.  
   
   <img width="869" alt="Screen Shot 2021-10-03 at 7 38 42 PM" src="https://user-images.githubusercontent.com/4567245/135785454-d90e1423-d52d-47a5-857e-6483298bcd7b.png">
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] john-bodley merged pull request #16950: other(revert): "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
john-bodley merged pull request #16950:
URL: https://github.com/apache/superset/pull/16950


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] edited a comment on pull request #16950: other(revert): "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16950:
URL: https://github.com/apache/superset/pull/16950#issuecomment-933102835


   # [Codecov](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#16950](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e7460fd) into [master](https://codecov.io/gh/apache/superset/commit/792efefcb4b475d657825bb4db174bcac82f0161?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (792efef) will **decrease** coverage by `0.21%`.
   > The diff coverage is `100.00%`.
   
   > :exclamation: Current head e7460fd differs from pull request most recent head d47b34c. Consider uploading reports for the commit d47b34c to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/16950/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #16950      +/-   ##
   ==========================================
   - Coverage   76.96%   76.74%   -0.22%     
   ==========================================
     Files        1038     1038              
     Lines       55612    55611       -1     
     Branches     7590     7590              
   ==========================================
   - Hits        42800    42678     -122     
   - Misses      12562    12683     +121     
     Partials      250      250              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.93% <100.00%> (-0.01%)` | :arrow_down: |
   | postgres | `81.94% <100.00%> (-0.01%)` | :arrow_down: |
   | presto | `?` | |
   | python | `82.03% <100.00%> (-0.42%)` | :arrow_down: |
   | sqlite | `81.62% <100.00%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `85.83% <100.00%> (-1.66%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.76% <0.00%> (-17.06%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.47% <0.00%> (-6.91%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `89.26% <0.00%> (-0.74%)` | :arrow_down: |
   | [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `88.20% <0.00%> (-0.39%)` | :arrow_down: |
   | [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.02% <0.00%> (-0.12%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [792efef...d47b34c](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] commented on pull request #16950: Revert "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16950:
URL: https://github.com/apache/superset/pull/16950#issuecomment-933102835


   # [Codecov](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#16950](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12ef016) into [master](https://codecov.io/gh/apache/superset/commit/ace9c786844a144c89e705c9ae244068c8692400?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ace9c78) will **decrease** coverage by `0.26%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/16950/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #16950      +/-   ##
   ==========================================
   - Coverage   76.98%   76.71%   -0.27%     
   ==========================================
     Files        1028     1028              
     Lines       55074    54964     -110     
     Branches     7486     7486              
   ==========================================
   - Hits        42396    42168     -228     
   - Misses      12430    12548     +118     
     Partials      248      248              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `?` | |
   | mysql | `81.83% <100.00%> (-0.01%)` | :arrow_down: |
   | postgres | `81.83% <100.00%> (-0.07%)` | :arrow_down: |
   | presto | `?` | |
   | python | `81.91% <100.00%> (-0.48%)` | :arrow_down: |
   | sqlite | `81.51% <100.00%> (-0.01%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `85.56% <100.00%> (-1.69%)` | :arrow_down: |
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-82.15%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.76% <0.00%> (-17.06%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.47% <0.00%> (-6.49%)` | :arrow_down: |
   | [superset/models/filter\_set.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2ZpbHRlcl9zZXQucHk=) | `75.43% <0.00%> (-3.03%)` | :arrow_down: |
   | [superset/models/dashboard.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2Rhc2hib2FyZC5weQ==) | `72.76% <0.00%> (-2.34%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
   | [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `75.97% <0.00%> (-0.94%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `89.26% <0.00%> (-0.74%)` | :arrow_down: |
   | ... and [16 more](https://codecov.io/gh/apache/superset/pull/16950/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ace9c78...12ef016](https://codecov.io/gh/apache/superset/pull/16950?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] john-bodley commented on a change in pull request #16950: other(revert): "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #16950:
URL: https://github.com/apache/superset/pull/16950#discussion_r732263165



##########
File path: superset/connectors/sqla/models.py
##########
@@ -334,8 +334,7 @@ def dttm_sql_literal(
         ],
     ) -> str:
         """Convert datetime object to a SQL expression string"""
-        dttm_type = self.type or ("DATETIME" if self.is_dttm else None)
-        sql = self.db_engine_spec.convert_dttm(dttm_type, dttm) if dttm_type else None
+        sql = self.db_engine_spec.convert_dttm(self.type, dttm) if self.type else None

Review comment:
       @villebro the issue is any column which is flagged as being `is_dttm` without a type is treated as a `DATETIME` type which could result in the wrong conversion.
   
   Again it seems like the original change was unnecessary and causes issues. The actual solution/paved path workflow—which worked previously—is merely to define the type of the calculated column. We likely should make this a required field and perform form validation.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] john-bodley commented on a change in pull request #16950: other(revert): "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #16950:
URL: https://github.com/apache/superset/pull/16950#discussion_r732263165



##########
File path: superset/connectors/sqla/models.py
##########
@@ -334,8 +334,7 @@ def dttm_sql_literal(
         ],
     ) -> str:
         """Convert datetime object to a SQL expression string"""
-        dttm_type = self.type or ("DATETIME" if self.is_dttm else None)
-        sql = self.db_engine_spec.convert_dttm(dttm_type, dttm) if dttm_type else None
+        sql = self.db_engine_spec.convert_dttm(self.type, dttm) if self.type else None

Review comment:
       @villebro the issue is any column which is flagged as being `is_dttm` without a type is treated as a `DATETIME` type which could result in the wrong conversion. It's not clear that to me that `DATETIME` is the best default, for example at Airbnb the vast majority of temporal columns are actually `VARCHAR`/`STRING` due to Hive partitions (which I _believe_ need to be strings).
   
   Again it seems like the original change was unnecessary and causes issues. The actual solution/paved path workflow—which worked previously—is merely to define the type of the calculated column. We likely should make this a required field and perform form validation.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] john-bodley commented on a change in pull request #16950: other(revert): "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #16950:
URL: https://github.com/apache/superset/pull/16950#discussion_r732263165



##########
File path: superset/connectors/sqla/models.py
##########
@@ -334,8 +334,7 @@ def dttm_sql_literal(
         ],
     ) -> str:
         """Convert datetime object to a SQL expression string"""
-        dttm_type = self.type or ("DATETIME" if self.is_dttm else None)
-        sql = self.db_engine_spec.convert_dttm(dttm_type, dttm) if dttm_type else None
+        sql = self.db_engine_spec.convert_dttm(self.type, dttm) if self.type else None

Review comment:
       @villebro the issue is any column which is flagged as being `is_dttm` without  a type is treated as a `DATETIME` type which could result in the wrong conversion.
   
   Again it seems like the fix was unnecessary as the actual solution/paved path workflow is merely to define the type of the calculated column. We likely should make this a required field and perform form validation.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a change in pull request #16950: other(revert): "fix: the calculated columns explicit type convert into date"

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16950:
URL: https://github.com/apache/superset/pull/16950#discussion_r726814090



##########
File path: superset/connectors/sqla/models.py
##########
@@ -334,8 +334,7 @@ def dttm_sql_literal(
         ],
     ) -> str:
         """Convert datetime object to a SQL expression string"""
-        dttm_type = self.type or ("DATETIME" if self.is_dttm else None)
-        sql = self.db_engine_spec.convert_dttm(dttm_type, dttm) if dttm_type else None
+        sql = self.db_engine_spec.convert_dttm(self.type, dttm) if self.type else None

Review comment:
       What types of problems did the original change cause? I agree that the assumption that a column is `DATETIME` when the datatype is undefined and `is_dttm` is `True` may be an oversimplification in some cases, but I would expect it's probably usually the most correct assumption, as I think many users don't bother populating the correct datatype in the widget.
   
   Long term, maybe we should execute a query with the calculated column to see what datatype the database returns and use that to populate the value if it's undefined when saving the dataset (probably quite a bit of work, but would probably solve this problem).




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org