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

SQL: when GROUPed BY, the DAY_NAME function outputs incorrect results #42556

Conversation

emasab
Copy link
Contributor

@emasab emasab commented May 25, 2019

- added unit test
@cbuescher cbuescher added the :Analytics/SQL SQL querying label May 27, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@matriv matriv self-assigned this Jun 3, 2019
@emasab
Copy link
Contributor Author

emasab commented Jun 18, 2019

hi, could you take a look at the PR?

@astefan
Copy link
Contributor

astefan commented Sep 6, 2019

@emasab I took a look at your PR and then analyzed the issue itself in more depth performing more tests following the investigation. It turned out that DAY_NAME wasn't the only function exhibiting the faulty behavior and a more general fix was needed. As such, I created this PR including a fix. As you can see, more classes needed the change and, also:

  • there is existing code that already has most of the functionality that you included in your PR, namely ScriptWeaver interface
  • the tests you added should have been placed in a different class - QueryTranslatorTests
  • I would have liked to see more integration tests, as well. See my PR with some examples.

We really appreciate you took the time to provide a fix and tests for it (and are looking forward to more PRs), but the actual change needed to fix the issue is quite different than what you have in this PR and, if you don't mind, I'll close your PR.

@astefan astefan closed this Sep 6, 2019
@emasab
Copy link
Contributor Author

emasab commented Sep 6, 2019

@astefan I've not tested your PR but given that it's a more complete fix, it's ok to close this one.

@matriv matriv removed their assignment Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/SQL SQL querying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants