[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.offset() and Table.fetch(). #4813

fhueske · 2017-10-12T09:09:01Z

What is the purpose of the change

Table.limit(n) has unexpected semantics of returning all but the first n rows.
Returning the first n rows from a sorted result requires to specify a 0 offset (table.orderBy(...).limit(0, n)) which is more complicated than necessary.

Brief change log

deprecate Table.limit() and replace it by Table.offset() and Table.fetch()

Verifying this change

existing tests for limit were adapted for offset and fetch.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no
The serializers: no
The runtime per-record code paths (performance sensitive): no
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no

Documentation

Documentation was adapted.

hustfxj · 2017-10-12T11:07:57Z

...le/src/test/scala/org/apache/flink/table/api/batch/table/validation/SortValidationTest.scala

@@ -27,10 +27,26 @@ import org.junit._
 class SortValidationTest extends TableTestBase {


It had better add the following two validation tests:

ds.orderBy('a.asc).offset(10).offset(10) ds.orderBy('a.asc).fetch(5)

Now fetch can only be used after offset, maybe the fetch can be used after orderBy. Then ds.orderBy('a.asc).fetch(5) is equivalent with ds.orderBy('a.asc).offset(0)fetch(5)

Right, I'll add a test that validates that table.orderBy('a.asc).offset(10).offset(10) is rejected.

table.orderBy('a.asc).fetch(5) is already supported as you suggested (see the modified SortITCase).

…offset() and Table.fetch().

xccui

Thanks for the PR @fhueske. It looks good. I just left some duplicated minor comments about the semantics.

Thanks, Xingcan

xccui · 2017-10-23T23:46:31Z

docs/dev/table/tableApi.md

+// returns all records beginning with the 4th record from the sorted result
+Table result2 = in.orderBy("a.asc").offset(3);
+
+// returns the first 5 records beginning with the 10th record from the sorted result


Should be 11th?

No it must be 10th, because SQL indices start at 1.

I think @xccui is right. An offset of 10 means that the first 10 rows are omitted from the result.
Otherwise offset 1 would not offset.
I'll update the docs.

xccui · 2017-10-23T23:47:08Z

docs/dev/table/tableApi.md

+// returns all records beginning with the 4th record from the sorted result
+val result2: Table = in.orderBy('a.asc).offset(3)
+
+// returns the first 5 records beginning with the 10th record from the sorted result


Should be 11th?

xccui · 2017-10-23T23:52:54Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/table.scala

+    * thus must be preceded by it.
+    *
+    * [[Table.offset(o)]] can be combined with a subsequent [[Table.fetch(n)]] call to return the
+    * first n rows starting from the offset position o.


If I understand correctly, "from the offset position o" means "from the (o+1)th record", right?

Maybe you can add a notice about the SQL indices here, as it might confuses users. Or you add skips 3 rows and begins with the 4th record.

xccui · 2017-10-23T23:53:16Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/table.scala

+    * {{{
+    *   // returns unlimited number of records beginning with the 4th record
+    *   tab.orderBy('name.desc).offset(3)
+    *   // return the first 5 records starting from the 10th record


xccui · 2017-10-23T23:55:14Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/table.scala

+    * {{{
+    *   // returns the first 3 records.
+    *   tab.orderBy('name.desc).fetch(3)
+    *   // return the first 5 records starting from the 10th record


xccui · 2017-10-24T00:13:05Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/table.scala

+    *
+    * @param fetch the number of records to return
+    */
+  def fetch(fetch: Int): Table = {


Since fetch: Int < 0 is allowed, maybe we should explicitly define the semantics of it?

Good point, thanks @xccui

twalthr

Thanks @fhueske. I also added some comments.

twalthr · 2017-10-24T08:38:39Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/table.scala

    */
+  @deprecated(message = "Deprecated in favor of Table.offset() and Table.fetch()", since = "1.4.0")


This kind of deprecation does not work for Java. Please add the Java annotation as well.

twalthr · 2017-10-24T08:40:24Z

docs/dev/table/tableApi.md

+// returns all records beginning with the 4th record from the sorted result
+Table result2 = in.orderBy("a.asc").offset(3);
+
+// returns the first 5 records beginning with the 10th record from the sorted result


No it must be 10th, because SQL indices start at 1.

twalthr · 2017-10-24T08:41:34Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/table.scala

+    * thus must be preceded by it.
+    *
+    * [[Table.offset(o)]] can be combined with a subsequent [[Table.fetch(n)]] call to return the
+    * first n rows starting from the offset position o.


Maybe you can add a notice about the SQL indices here, as it might confuses users. Or you add skips 3 rows and begins with the 4th record.

fhueske · 2017-10-24T12:26:38Z

Thanks for the reviews @xccui and @twalthr.
I'll address the feedback and merge this PR.

…offset() and Table.fetch(). This closes apache#4813.

hustfxj reviewed Oct 12, 2017

View reviewed changes

[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.…

a9dd5b6

…offset() and Table.fetch().

fhueske force-pushed the tableOffsetFetch branch from 7b1e3cb to a9dd5b6 Compare October 12, 2017 12:05

xccui reviewed Oct 24, 2017

View reviewed changes

twalthr requested changes Oct 24, 2017

View reviewed changes

fhueske added a commit to fhueske/flink that referenced this pull request Oct 24, 2017

[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.…

627372e

…offset() and Table.fetch(). This closes apache#4813.

asfgit closed this in e1c7e49 Oct 24, 2017

fhueske deleted the tableOffsetFetch branch October 24, 2017 18:41

rmetzger added the component=API/TableSQL label Mar 18, 2019

flinkbot added component=TableSQL/API and removed component=API/TableSQL labels Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.offset() and Table.fetch(). #4813

[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.offset() and Table.fetch(). #4813

fhueske commented Oct 12, 2017

hustfxj Oct 12, 2017 •

edited

Loading

fhueske Oct 12, 2017

xccui left a comment

xccui Oct 23, 2017

twalthr Oct 24, 2017

fhueske Oct 24, 2017

xccui Oct 23, 2017

xccui Oct 23, 2017

twalthr Oct 24, 2017

fhueske Oct 24, 2017

xccui Oct 23, 2017

xccui Oct 23, 2017

xccui Oct 24, 2017

fhueske Oct 24, 2017

twalthr left a comment

twalthr Oct 24, 2017

twalthr Oct 24, 2017

twalthr Oct 24, 2017

fhueske commented Oct 24, 2017

		@@ -27,10 +27,26 @@ import org.junit._
		class SortValidationTest extends TableTestBase {

		*/
		@deprecated(message = "Deprecated in favor of Table.offset() and Table.fetch()", since = "1.4.0")

[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.offset() and Table.fetch(). #4813

[FLINK-7821] [table] Deprecate Table.limit() and replace it by Table.offset() and Table.fetch(). #4813

Conversation

fhueske commented Oct 12, 2017

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

hustfxj Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xccui left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twalthr left a 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

fhueske commented Oct 24, 2017

hustfxj Oct 12, 2017 •

edited

Loading