Deprecate using "interval" configuration for date histograms aggregations #1874

romainneutron · 2020-11-23T10:39:22Z

The combined interval field has been deprecated in favor of fixed_interval or calendar_interval. See https://www.elastic.co/guide/en/elasticsearch/reference/7.2/search-aggregations-bucket-datehistogram-aggregation.html

deguif

👍

src/QueryBuilder/DSL/Aggregation.php

deguif · 2020-11-23T10:57:05Z

Thanks @romainneutron
Would you be able to add a unit test to ensure deprecation is triggered? See https://github.com/ruflin/Elastica/blob/master/tests/IndexTest.php#L604 for example

src/Aggregation/Histogram.php

tests/Aggregation/DateHistogramTest.php

deguif · 2020-11-23T13:23:41Z

src/Aggregation/DateHistogram.php

@@ -7,10 +7,67 @@
 *
 * @see https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-datehistogram-aggregation.html
 */
-class DateHistogram extends Histogram
+class DateHistogram extends AbstractSimpleAggregation


That would be a BC break.

I thought BC breaks were allowed

Sometimes but should not ;)
I think in this case, we can avoid it or I'm missing something?

so, what should we do?

Wouldn't this be helpful (#1874 (comment)) to avoid changing the inheritance?

Ok I think I got the point.
DateHistogram interval parameter is deprecated but still useful for Histogram.
Can you confirm it's what you're addressing by changing the inheritance chain?

yes. We could move methods if we want to keep those implementation.
Maintaining BC on interfaces and userland API yes, but should we maintain BC on abstract internal classes?

deguif · 2020-11-23T13:47:20Z

tests/Aggregation/DateHistogramTest.php

+        $this->_checkVersion('7.2');
+
+        $agg = new DateHistogram('hist', 'created');
+        $agg->setFixedInterval('1h');


Suggested change

$this->_checkVersion('7.2');

$agg = new DateHistogram('hist', 'created');

$agg->setFixedInterval('1h');

$version = $this->_getVersion();

if (\version_compare($version, '7.2') < 0) {

$this->expectDeprecation('Since ruflin/elastica 7.1.0: The "Elastica\Aggregation\DateHistogram::setInterval()" method is deprecated, use "setDateInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.');

$agg = (new DateHistogram('hist', 'created'))

->setInterval('1h')

;

} else {

$agg = (new DateHistogram('hist', 'created'))

->setFixedInterval('1h')

;

}

Cannot we use a code like this to test on version lower than 7.2?

Suggested change

$this->_checkVersion('7.2');

$agg = new DateHistogram('hist', 'created');

$agg->setFixedInterval('1h');

$version = $this->_getVersion();

if (\version_compare($version, '7.2') < 0) {

$agg = (new DateHistogram('hist', 'created'))

->setParam('interval', '1h')

;

} else {

$agg = (new DateHistogram('hist', 'created'))

->setFixedInterval('1h')

;

}

Or like this?

Maybe the deprecation is a little too violent, as this can't be fixed for versions lower than 7.2 unless using setParam() directly? WDYT @ruflin @thePanz ? Is there any advices/hints for this kind of deprecations?

Not sure if it's a blocker as the deprecation is also very helpful for upper compatibility.

romainneutron · 2020-11-23T18:31:53Z

PR udpated.
I dont think we should consider still consider using Histogram as base for DateHistogram. They're both histogram but differ on some of their behavior.

ruflin · 2020-11-25T08:29:27Z

If understand it there are 2 things here:

Changing the inheritance of DateHistogram
Deprecating interval

We need the first one to only deprecate interval for DateHistogram?

It seems the current code change is backward compatible expect in the case someone checked for specifically inherited classes? Is there more the could break users code?

romainneutron · 2020-12-07T16:02:05Z

PR rebased

ruflin · 2020-12-08T14:15:01Z

Seems like one of the tests fail because of the deprecation :-) Can you fix it and also rebased the changelog?

romainneutron · 2020-12-09T17:45:02Z

PR rebased, tests are green :)

ruflin · 2020-12-14T07:24:29Z

I'll refer to @deguif on the final 👍 as he had the most interactions here.

romainneutron · 2021-01-04T13:04:47Z

any news?

ruflin · 2021-01-04T14:31:52Z

@deguif Any chance to have a quick look?

deguif · 2021-01-06T12:56:22Z

src/Aggregation/DateHistogram.php

+        $this->setField($field);
+
+        if (null !== $interval) {
+            trigger_deprecation('ruflin/elastica', '7.1.0', 'Argument 3 passed to "%s()" is deprecated, use "setDateInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.', __METHOD__);


Suggested change

trigger_deprecation('ruflin/elastica', '7.1.0', 'Argument 3 passed to "%s()" is deprecated, use "setDateInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.', __METHOD__);

trigger_deprecation('ruflin/elastica', '7.1.0', 'Argument 3 passed to "%s()" is deprecated, use "setFixedInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.', __METHOD__);

deguif · 2021-01-06T13:00:18Z

tests/Aggregation/BucketSelectorTest.php

@@ -46,9 +46,12 @@ public function testToArray(): void
     */
    public function testMaxAggregation(): void
    {
+        $this->_checkVersion('7.2');


Cannot we do the same you've done later for DateHistogramTest::testDateHistogramAggregation(), I mean checking the version of elasticsearch and using setInterval() or setFixedInterval() depending the version?

deguif · 2021-01-06T13:02:25Z

tests/Aggregation/DerivativeTest.php

@@ -45,9 +45,12 @@ public function testToArray(): void
     */
    public function testMaxAggregation(): void
    {
+        $this->_checkVersion('7.2');


Same here:

Cannot we do the same you've done later for DateHistogramTest::testDateHistogramAggregation(), I mean checking the version of elasticsearch and using setInterval() or setFixedInterval() depending the version?

deguif · 2021-01-06T13:02:38Z

tests/Aggregation/SerialDiffTest.php

@@ -20,7 +20,10 @@ class SerialDiffTest extends BaseAggregationTest
     */
    public function testSerialDiffAggregation(): void
    {
-        $dateHistogramAggregation = new DateHistogram('measurements', 'measured_at', 'hour');
+        $this->_checkVersion('7.2');


Same here:

Cannot we do the same you've done later for DateHistogramTest::testDateHistogramAggregation(), I mean checking the version of elasticsearch and using setInterval() or setFixedInterval() depending the version?

deguif · 2021-01-06T13:11:14Z

src/Aggregation/DateHistogram.php

+     *
+     * @return $this
+     */
+    public function setOrder(string $order, string $direction): self


It seems it can also be a multi-dimensional array?
Like this:

"order": [ { "rock>playback_stats.avg": "desc" }, { "_count": "desc" } ]

deguif · 2021-01-06T13:21:25Z

src/Aggregation/DateHistogram.php

+     */
+    public function setInterval($interval): self
+    {
+        trigger_deprecation('ruflin/elastica', '7.1.0', 'The "%s()" method is deprecated, use "setDateInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.', __METHOD__);


I'm dubious about this deprecation, as interval parameter is still legit for ES version lower than 7.2. It was only deprecated in 7.2 version but is still supported (until at least 8.0).

I think we should remove the deprecation here. But I would let the deprecation in the constructor so that users would be required to update their code to explicitly call setInterval(), setFixedInterval() or setCalendarInterval().

Later when bumping minimum version of ES to a version later than 7.2 (probably when bumping to 8.0) we should be able to drop this method and only let the two others.

Would that make sense? ping @ruflin @thePanz

There are two different deprecations:

Deprecation in Elasticsearch

Deprecation in Elastica

In the best case, they are aligned. As Elastica lags behind in releases, we kind of have the option to deprecate things already in older releases, assuming there is already an option available which is not deprecated.

To remove a method in 8.0, we should have it deprecated already in 7.x. I would say the earlier we can tell devs to adjust their code the better.

Yes but that means in this case, that users of version older than 7.2 will get this deprecation but won't be able to remove the deprecation as the parameters (setFixedInterval, setCalendarInterval) can't work with their ES version. But maybe that's not an issue.

I see. Maybe we can leave in a note in the docs for this. In general it would be great if we can push users to use the newest version of Elastica and the related version of Elasticsearch and would optimise for this. Maybe we could even adjust the README that this version of Elastica is supported for Elasticsearch >= 7.3 or similar. It does not mean, it will not work anymore with the older versions.

I would add the PHP Doc to the methods, stating that those are not supported by ES >= 8.x
When the library will support ES v8.x only, we can remove those methods.
WDYT?

No strong preference on my end. SGTM

VincentLanglet · 2022-07-11T10:51:01Z

This #2099 could be a simpler way to do it, isn't it ?

thePanz · 2022-08-02T12:48:20Z

Fixed in #2099

romainneutron force-pushed the add-interval-deprecation branch from d433024 to 05805f5 Compare November 23, 2020 10:44

deguif approved these changes Nov 23, 2020

View reviewed changes

src/QueryBuilder/DSL/Aggregation.php Show resolved Hide resolved

romainneutron force-pushed the add-interval-deprecation branch from 05805f5 to dfb07a3 Compare November 23, 2020 10:49

deguif reviewed Nov 23, 2020

View reviewed changes

src/Aggregation/Histogram.php Outdated Show resolved Hide resolved

romainneutron force-pushed the add-interval-deprecation branch 8 times, most recently from f8e4111 to 5b97e70 Compare November 23, 2020 12:05

romainneutron commented Nov 23, 2020

View reviewed changes

tests/Aggregation/DateHistogramTest.php Outdated Show resolved Hide resolved

romainneutron changed the title ~~Deprecate using "interval" configuration for histograms aggregations~~ Deprecate using "interval" configuration for date histograms aggregations Nov 23, 2020

deguif requested changes Nov 23, 2020

View reviewed changes

deguif reviewed Nov 23, 2020

View reviewed changes

romainneutron force-pushed the add-interval-deprecation branch 2 times, most recently from 2747dac to 4fcbfe1 Compare November 23, 2020 18:26

romainneutron force-pushed the add-interval-deprecation branch 2 times, most recently from 785d5bc to 2831bd5 Compare November 23, 2020 18:35

romainneutron force-pushed the add-interval-deprecation branch from 2831bd5 to b0f0deb Compare December 7, 2020 16:01

romainneutron force-pushed the add-interval-deprecation branch from b0f0deb to af82aae Compare December 7, 2020 16:15

romainneutron force-pushed the add-interval-deprecation branch from af82aae to 32e09c2 Compare December 9, 2020 17:23

Deprecate using "interval" configuration for date-histograms aggregation

663cf58

romainneutron force-pushed the add-interval-deprecation branch from 32e09c2 to 663cf58 Compare December 9, 2020 17:40

ruflin requested a review from deguif December 10, 2020 13:53

deguif reviewed Jan 6, 2021

View reviewed changes

deguif mentioned this pull request Jan 18, 2021

Adding new ElasticSearch 7.10 'case_insensitive' param to Wildcard. #1894

Merged

VincentLanglet mentioned this pull request Jul 12, 2022

Avoid deprecation with interval for DateHistogram #2099

Closed

thePanz closed this Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate using "interval" configuration for date histograms aggregations #1874

Deprecate using "interval" configuration for date histograms aggregations #1874

romainneutron commented Nov 23, 2020 •

edited

deguif left a comment

deguif commented Nov 23, 2020

deguif Nov 23, 2020

romainneutron Nov 23, 2020

deguif Nov 23, 2020

romainneutron Nov 23, 2020

deguif Nov 23, 2020

deguif Nov 23, 2020

romainneutron Nov 23, 2020

deguif Nov 23, 2020

deguif Nov 23, 2020

deguif Nov 23, 2020

romainneutron commented Nov 23, 2020

ruflin commented Nov 25, 2020

romainneutron commented Dec 7, 2020

ruflin commented Dec 8, 2020

romainneutron commented Dec 9, 2020

ruflin commented Dec 14, 2020

romainneutron commented Jan 4, 2021

ruflin commented Jan 4, 2021 •

edited

deguif Jan 6, 2021

deguif Jan 6, 2021

deguif Jan 6, 2021

deguif Jan 6, 2021

deguif Jan 6, 2021

deguif Jan 6, 2021

ruflin Jan 7, 2021

deguif Jan 17, 2021

ruflin Jan 18, 2021

thePanz Jan 18, 2021

ruflin Jan 19, 2021

VincentLanglet commented Jul 11, 2022

thePanz commented Aug 2, 2022

-        $this->_checkVersion('7.2');
-        $agg = new DateHistogram('hist', 'created');
-        $agg->setFixedInterval('1h');
+        $version = $this->_getVersion();
+        if (\version_compare($version, '7.2') < 0) {
+            $this->expectDeprecation('Since ruflin/elastica 7.1.0: The "Elastica\Aggregation\DateHistogram::setInterval()" method is deprecated, use "setDateInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.');
+            $agg = (new DateHistogram('hist', 'created'))
+                ->setInterval('1h')
+            ;
+        } else {
+            $agg = (new DateHistogram('hist', 'created'))
+                ->setFixedInterval('1h')
+            ;
+        }

	trigger_deprecation('ruflin/elastica', '7.1.0', 'Argument 3 passed to "%s()" is deprecated, use "setDateInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.', __METHOD__);
	trigger_deprecation('ruflin/elastica', '7.1.0', 'Argument 3 passed to "%s()" is deprecated, use "setFixedInterval()" or "setCalendarInterval()" instead. It will be removed in 8.0.', __METHOD__);

Deprecate using "interval" configuration for date histograms aggregations #1874

Deprecate using "interval" configuration for date histograms aggregations #1874

Conversation

romainneutron commented Nov 23, 2020 • edited

deguif left a comment

Choose a reason for hiding this comment

deguif commented Nov 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romainneutron commented Nov 23, 2020

ruflin commented Nov 25, 2020

romainneutron commented Dec 7, 2020

ruflin commented Dec 8, 2020

romainneutron commented Dec 9, 2020

ruflin commented Dec 14, 2020

romainneutron commented Jan 4, 2021

ruflin commented Jan 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Jul 11, 2022

thePanz commented Aug 2, 2022

romainneutron commented Nov 23, 2020 •

edited

ruflin commented Jan 4, 2021 •

edited