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

Send date_histogram extended_bounds as a long. Closes #1868 #1967

Merged
merged 3 commits into from
Nov 19, 2014

Conversation

rashidkpc
Copy link
Contributor

Fixes extended_bounds in date_histogram agg for for users that have non-ISO8601 timestamps

@rashidkpc rashidkpc added this to the 4.0.0-BETA3 milestone Nov 18, 2014
@w33ble
Copy link
Contributor

w33ble commented Nov 19, 2014

Seems right to me based on the issue it's closing and the parent of that. I'm not super familiar with extended_bounds though, and the guide doesn't make it clear what the min and max values may end up being. That said, I'm going to ask that someone else who does know this review as well.

And, one comment on the test - it's worth passing in values for min and max instead of just the moment object, as I believe that's more the "norm" (based on the docs). Making a test like so might be useful:

describe('extended_bounds', function () {
        it('should write the long value of the moment passed in', function () {
          var min = 0;
          var max = 500;
          var output = paramWriter.write({
            extended_bounds: {
              min: min,
              max: max
            }
          });
          expect(output.params.extended_bounds.min).to.be(moment(min).valueOf());
          expect(output.params.extended_bounds.max).to.be(moment(max).valueOf());
        });
      });

@w33ble w33ble assigned spalger and unassigned w33ble Nov 19, 2014
@rashidkpc
Copy link
Contributor Author

min/max represent dates, that the only safe way to pass dates to Elasticsearch, without knowing the stored format, is as a long.

The reason I changed the original code to wrap the values in moment() was just in case something else passes in a long, I'll add a test for it as well per your suggestion.

@spalger
Copy link
Contributor

spalger commented Nov 19, 2014

LGTM

spalger pushed a commit that referenced this pull request Nov 19, 2014
Send date_histogram extended_bounds as a long. Closes #1868
@spalger spalger merged commit cffbb9e into elastic:master Nov 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants