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 2020/06/17 11:21:40 UTC
[GitHub] [incubator-superset] Nj-kol opened a new pull request #10084: Fixed bug for issue #9967
Nj-kol opened a new pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084
### SUMMARY
The issue was first observed in https://github.com/apache/incubator-superset/issues/7270
However, in the subsequent releases, the variable **time_grain_functions** was renamed to **_time_grain_expressions**. The implementation was also left out for hive. So time functions were not renamed which were specific to Hive. This was causing the group by functions in the Charts to malfunction
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
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.
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] [incubator-superset] 0xBADBAC0N commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
0xBADBAC0N commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r446075194
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -16,6 +16,10 @@
# under the License.
from unittest import mock
+from pyhive import hive
+from sqlalchemy import column
+from sqlalchemy.dialects import *
Review comment:
Hi,
is there a specific reason to have an import star? Otherwise, I personally would prefer single imports :)
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r449395023
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -16,6 +16,10 @@
# under the License.
from unittest import mock
+from pyhive import hive
+from sqlalchemy import column
+from sqlalchemy.dialects import *
Review comment:
Right, and that very question proves the point of how hard it is to track what's behind the `*`
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-655308908
Not sure how got tangled but I'd suggest an interactive rebase to get rid of the commits that don't belong here. The short story is `git rebase -i {SHA_OF_YOUR_FIRST_COMMIT_IN_THIS_BRANCH}` and then you remove the lines for the commits you don't want in the text editor that pops up.
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645395995
@dpgaspar Could you please tell me where to add these tests ( ```get_timestamp_expr``` ). Apologies for the NOOB questions, I made this emergency fix and just about got this working. Unfortunately, I am new to python as well as a Superset. I may need some help from the community for this. :)
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol edited a comment on pull request #10084: Fixed bug for issue #9967
Posted by GitBox <gi...@apache.org>.
Nj-kol edited a comment on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645337588
@villebro Thanks for your feedback. I ran the black formatted as per your suggestion, but still, this seems to be failing. The problem is I am new to python, so I think I may be missing something here. I think I may require some help here.
----------------------------------------------------------------
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.
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] [incubator-superset] john-bodley commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
john-bodley commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-646732497
@Nj-kol you can run the tests locally (scoped to the specific file, class, or test) using tox, i.e.,
```
tox -e py36-sqlite -- tests/db_engine_specs/hive_tests.py
```
which is documented in more detail [here](https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#python-testing). Note per the `tox` configuration development requirements are [installed](https://github.com/apache/incubator-superset/blob/master/tox.ini#L25) which include [`pyhive`](https://github.com/apache/incubator-superset/blob/master/requirements-dev.txt#L30).
----------------------------------------------------------------
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.
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] [incubator-superset] john-bodley commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r442649185
##########
File path: scripts/permissions_cleanup.py
##########
@@ -39,7 +39,7 @@ def cleanup_permissions():
sm.get_session.commit()
pvms = sm.get_session.query(sm.permissionview_model).all()
- print('STage 1: # of permission view menues is: {}'.format(len(pvms)))
+ print("STage 1: # of permission view menues is: {}".format(len(pvms)))
Review comment:
Nit. Could you also rename `STage` to `Stage`.
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645825597
@dpgaspar I have added tests to``hive_tests.py``. But it fails because the *pyhive* module is missing. How do I enable this?
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-656047180
@Nj-kol a quick fix for this would be opening a new PR with just a single commit that includes the relevant changes. I'd like to cut the first release candidate for 0.37.0 today, and this would be great to get into that release.
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r450630971
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -233,4 +233,4 @@ def test_get_create_table_stmt() -> None:
STORED AS TEXTFILE LOCATION :location
tblproperties ('skip.header.line.count'=':header_line_count')""",
{"delim": ",", "location": "s3a://directory/table", "header_line_count": "101"},
- )
+ )
Review comment:
I didn't add anything here but took whatever is there in the master branch : https://github.com/apache/incubator-superset/blob/master/tests/db_engine_specs/hive_tests.py
Please do let me know what I need to modify, I'll make the same
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654607708
@villebro I have rebased the code, and removed the tests. Could you please have a look?
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-656064440
Great, thanks! 👍
----------------------------------------------------------------
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.
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] [incubator-superset] villebro closed pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro closed pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r449394054
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -153,6 +157,20 @@ def test_hive_get_view_names_return_empty_list(
[], HiveEngineSpec.get_view_names(mock.ANY, mock.ANY, mock.ANY)
)
+ def test_time_exp_mixd_case_col_1y(self):
+ # pylint: disable=line-too-long
+ # pylint: disable=invalid-name
+ """
+ DB Eng Specs (hive): Test grain expr mixed case 1 YEAR
+ """
+ col = column("MixedCase")
+ expr = HiveEngineSpec.get_timestamp_expr(col, None, "P1Y")
+ result = str(expr.compile(None, dialect=hive.dialect()))
Review comment:
I'm not sure how we've usually handled Sql Alchemy plugin dialects in tests, but I think this needs to check if `hive` is available, and only run the test if that is the case.
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -16,6 +16,10 @@
# under the License.
from unittest import mock
+from pyhive import hive
+from sqlalchemy import column
+from sqlalchemy.dialects import *
Review comment:
Is this import really needed? It seems none of the standard Sql Alchemy dialects are used here.
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r450635801
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -233,4 +233,4 @@ def test_get_create_table_stmt() -> None:
STORED AS TEXTFILE LOCATION :location
tblproperties ('skip.header.line.count'=':header_line_count')""",
{"delim": ",", "location": "s3a://directory/table", "header_line_count": "101"},
- )
+ )
Review comment:
@villebro Added the new line
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r442651209
##########
File path: scripts/permissions_cleanup.py
##########
@@ -39,7 +39,7 @@ def cleanup_permissions():
sm.get_session.commit()
pvms = sm.get_session.query(sm.permissionview_model).all()
- print('STage 1: # of permission view menues is: {}'.format(len(pvms)))
+ print("STage 1: # of permission view menues is: {}".format(len(pvms)))
Review comment:
Sure. Will do!
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: Fixed bug for issue #9967
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645337588
@villebro Thanks for your feedback, I ran the black formatted as per your suggestion, but still this seems to be failing. The problem is I am new to python, so I think I am missing something here. I think I may require some help hee
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654886617
I think something may have gone wrong with your rebase, there's lots of unrelated changes here now.
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r450629902
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -233,4 +233,4 @@ def test_get_create_table_stmt() -> None:
STORED AS TEXTFILE LOCATION :location
tblproperties ('skip.header.line.count'=':header_line_count')""",
{"delim": ",", "location": "s3a://directory/table", "header_line_count": "101"},
- )
+ )
Review comment:
Didn't catch that, do you want me to add a new line at the end?
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654004803
> @Nj-kol this fix would be important to get merged before cutting the 0.37 release. If you feel uneasy adding the unit test, please feel free to remove the test so we can get this merged. I'll be happy to reintroduce the test later, as we need to address similar tests for other engines, too.
Yes. I have kind of been struggling with getting the tests to pass, largely because of my lack of experience in Python. If this is urgent and know can do without the tests, I'll remove them
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654629698
Something strange with CI, restarted.
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654659899
> @Nj-kol can you revert the changes in `scripts/permissions_cleanup.py` and push a new commit if that would help kickstart CI?
Didn't catch that. What do I need to change in the ```scripts/permissions_cleanup.py``` file?
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r450629154
##########
File path: requirements-dev.txt
##########
@@ -31,6 +31,7 @@ pycodestyle==2.5.0
pydruid==0.6.1
pyhive==0.6.2
pylint==2.5.3
+ded thrift dependency
Review comment:
Whoah! That's a good catch, yes that's a typo. I'll fix it
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-656102767
> Great, thanks! 👍
Created new PR: https://github.com/apache/incubator-superset/pull/10269
But still can't get the CI to pass :(
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-646476340
Need help on how to include ```pyhive``` dependency in tests. I am importing the ```pyhive``` module in ```hive_tests.py```. Also, I wanted to understand how to run the exact same CI pipeline for the tests on my local machine. I want to be sure that the test succeed locally before pushing the changed to remote
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654907852
TBH this looks like GitHub isn't working correctly. The unrelated changes are already merged on master, so they shouldn't show up as changes. Let's wait a while and see if GH snaps out of it in automatically.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654651178
@Nj-kol can you revert the changes in `scripts/permissions_cleanup.py` and push a new commit if that would help kickstart CI?
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-653937870
@Nj-kol this fix would be important to get merged before cutting the 0.37 release. If you feel uneasy adding the unit test, please feel free to remove the test so we can get this merged. I'll be happy to reintroduce the test later, as we need to address similar tests for other engines, too.
----------------------------------------------------------------
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.
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] [incubator-superset] dpgaspar commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645413786
No need to apologize, add tests here:
https://github.com/apache/incubator-superset/blob/master/tests/db_engine_specs/hive_tests.py
You can find examples for tests time grains here:
https://github.com/apache/incubator-superset/blob/master/tests/db_engine_specs/postgres_tests.py#L53
Thank you again for taking the time and effort!
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: Fixed bug for issue #9967
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645371005
@dpgaspar I'll have a look. Kindly ignore the last review request
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654914273
> TBH this looks like GitHub isn't working correctly. The unrelated changes are already merged on master, so they shouldn't show up as changes. Let's wait a while and see if GH snaps out of it in automatically.
Hmm, let's see. On the bright side, I see the CI pipeline working. FYI, I followed these steps for rebasing
```## Step 1 : Switch from the feature branch ( of pull request ) to master branch
git checkout master
## Step 2 : Add an upstream
git remote add upstream <upstreamrepourl>
Ex -
git remote add upstream https://github.com/apache/incubator-superset.git
* At this point, you'll have two remotes
git remote -v
```
origin https://github.com/Nj-kol/incubator-superset (fetch)
origin https://github.com/Nj-kol/incubator-superset (push)
upstream https://github.com/apache/incubator-superset.git (fetch)
upstream https://github.com/apache/incubator-superset.git (push)
```
## Step 3 : Rewrite your forked master branch
Any commits of yours that aren't already in ```upstream master``` are replayed on top of that other branch:
git pull --rebase upstream master
## Step 4 : Checkout PR branch
git checkout bugfix/#9967
## Rebase PR branch with forked master
git rebase master
## Resolve merge conflicts
* Resolve Conflicts from 16:00
* If there are a lot of merge conflicts, you can abort the rebase and go back using :
git rebase --abort
## Add the merge resolved files
git add tests/db_engine_specs/hive_tests.py
git add requirements-dev.txt
## Commit the changes
git commit -m "Some message"
## Rebase again
git rebase --continue
## Push changes from PR branch
git push --force
````
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654028751
@Nj-kol feel free to remove the tests (you also need to rebase). I'll add the tests once this is merged.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r450627712
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -233,4 +233,4 @@ def test_get_create_table_stmt() -> None:
STORED AS TEXTFILE LOCATION :location
tblproperties ('skip.header.line.count'=':header_line_count')""",
{"delim": ",", "location": "s3a://directory/table", "header_line_count": "101"},
- )
+ )
Review comment:
line change missing here
##########
File path: requirements-dev.txt
##########
@@ -31,6 +31,7 @@ pycodestyle==2.5.0
pydruid==0.6.1
pyhive==0.6.2
pylint==2.5.3
+ded thrift dependency
Review comment:
typo?
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654075613
> @Nj-kol feel free to remove the tests (you also need to rebase). I'll add the tests once this is merged.
I'll get on it. Thanks!
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654637491
Hmm, very strange, for some reason CI is still trying to install that broken dependency that was already removed in a more recent commit. Will restart once more, but it might require adding a new commit to kickstart it again.
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r448609942
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -16,6 +16,10 @@
# under the License.
from unittest import mock
+from pyhive import hive
+from sqlalchemy import column
+from sqlalchemy.dialects import *
Review comment:
+1, let's not `import *` as we can't trace where objects are coming from, otherwise LGTM
----------------------------------------------------------------
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.
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] [incubator-superset] mistercrunch commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645466832
Thanks for bearing with us, having tests will prevent the regression like the one that this PR is fixing!
Also about `black`, we recommend installing pre-commit hooks:
https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#git-hooks
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654639695
> Hmm, very strange, for some reason CI is still trying to install that broken dependency that was already removed in a more recent commit. Will restart once more, but it might require adding a new commit to kickstart it again.
Yeah! not sure why it's still failing
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-656064049
> a quick fix for this would be opening a new PR with just a single commit that includes the relevant changes. I'd like to cut the first release candidate for 0.37.0 today, and this would
Yes! I thought the same thing. I'll try for that then by today
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: Fixed bug for issue #9967
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645318112
Thanks @Nj-kol for fixing! Can you run `black` to fix linting errors?
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
Nj-kol commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654899178
> I think something may have gone wrong with your rebase, there's lots of unrelated changes here now.
Yeah, I see that too. Don't know how that happened. Is there any way to undo this?
----------------------------------------------------------------
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.
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] [incubator-superset] Nj-kol edited a comment on pull request #10084: Fixed bug for issue #9967
Posted by GitBox <gi...@apache.org>.
Nj-kol edited a comment on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645337588
@villebro Thanks for your feedback. I ran the black formatted as per your suggestion, but still this seems to be failing. The problem is I am new to python, so I think I am missing something here. I think I may require some help hee
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-654663680
If you look at the "Changes" tab, you'll see that there are changes to that file in this PR that aren't directly related to the fix.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on pull request #10084: Fixed bug for issue #9967
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#issuecomment-645344104
@Nj-kol the problem is that `pylint` expects lines to be no longer than 88 characters. You can either add a `# pylint: disable=line-too-long` (prefixed with two spaces) to the end of the lines that exceed 88 characters, or break the long expressions into multi-line strings. I personally prefer splitting across multiple rows if it doesn't make the expression less readable, but both are ok.
----------------------------------------------------------------
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.
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] [incubator-superset] villebro commented on a change in pull request #10084: fix: Fixed bug for issue #9967 ( Hive date functions for group functions in charts )
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10084:
URL: https://github.com/apache/incubator-superset/pull/10084#discussion_r450630950
##########
File path: tests/db_engine_specs/hive_tests.py
##########
@@ -233,4 +233,4 @@ def test_get_create_table_stmt() -> None:
STORED AS TEXTFILE LOCATION :location
tblproperties ('skip.header.line.count'=':header_line_count')""",
{"delim": ",", "location": "s3a://directory/table", "header_line_count": "101"},
- )
+ )
Review comment:
It seems the last line change was removed in this PR (the last line should always end in a line change). So just adding that should resolve the linting 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.
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