Skip to content

Conversation

@jsternberg
Copy link
Contributor

@jsternberg jsternberg commented May 9, 2022

The preview function will limit the number of rows and the number of
tables that can be returned by a transformation. It will discard any
remaining tables/rows.

Fixes #4719.

Done checklist

  • docs/SPEC.md updated
  • Test cases written

@jsternberg jsternberg requested review from a team as code owners May 9, 2022 21:12
@jsternberg jsternberg requested review from lwandzura and wolffcm and removed request for a team May 9, 2022 21:12
@sanderson sanderson requested review from sanderson and removed request for lwandzura May 9, 2022 21:12
Comment on lines 1256 to 1259
// preview limits the number of rows and tables in the stream.
// The group keys that are included are not deterministic and depends on the order
// that the engine sends them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// preview limits the number of rows and tables in the stream.
// The group keys that are included are not deterministic and depends on the order
// that the engine sends them.
// preview limits the number of rows and tables in the stream.
//
// Included groups keys are not deterministic and depend on the order
// that the engine sends them.

// that the engine sends them.
//
// ## Parameters
// - nrows: Maximum number of rows in each table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// - nrows: Maximum number of rows in each table.
// - nrows: Maximum number of rows per table to return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this parameter have a default value? The example below suggests that it does.

// ## Parameters
// - nrows: Maximum number of rows in each table.
//
// - ntables: Maximum number of tables returned by the transformation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// - ntables: Maximum number of tables returned by the transformation.
// - ntables: Maximum number of tables to return.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this parameter have a default value? The example below suggests that it does.

Comment on lines 1270 to 1275
// ```
// import "experimental"
// import "sampledata"
//
// sampledata.init()
// |> experimental.preview()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// ```
// import "experimental"
// import "sampledata"
//
// sampledata.init()
// |> experimental.preview()
// ```
// import "experimental"
// import "sampledata"
//
// sampledata.int()
// |> experimental.preview()
// ```

// |> experimental.preview()
//
// ## Metadata
// introduced: next
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathanielc Does case matter here? Does this need to be NEXT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's supposed to be NEXT. Fixed.

@jsternberg jsternberg force-pushed the feat/experimental-preview branch 3 times, most recently from d75e074 to 818f27a Compare May 9, 2022 21:21
Copy link

@wolffcm wolffcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

@jsternberg jsternberg force-pushed the feat/experimental-preview branch from 818f27a to 41449da Compare May 10, 2022 13:50
Comment on lines +1258 to +1259
// Included group keys are not deterministic and depends on the order
// that the engine sends them.
Copy link
Contributor

@sanderson sanderson May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Included group keys are not deterministic and depends on the order
// that the engine sends them.
// The function is designed to quickly provide a data preview by not
// returning the entire result set.
// Included group keys are not deterministic and depend on the order
// that the engine sends them.

Comment on lines 1276 to 1277
// sampledata.init()
// |> experimental.preview()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Change sampledata.init() to sampledata.int()
  • Passing parameters in the example will show exactly how the input tables are modified.
  • Add input (<) and output (>) identifiers to generate the input and output examples.
Suggested change
// sampledata.init()
// |> experimental.preview()
// < sampledata.int()
// > |> experimental.preview(nrows: 3, ntables: 1)

@jsternberg jsternberg force-pushed the feat/experimental-preview branch from 41449da to 334dbeb Compare May 10, 2022 15:05
The preview function will limit the number of rows and the number of
tables that can be returned by a transformation. It will discard any
remaining tables/rows.
@jsternberg jsternberg force-pushed the feat/experimental-preview branch from 334dbeb to d5ea073 Compare May 10, 2022 15:51
@jsternberg jsternberg merged commit 9d27bb9 into master May 10, 2022
@jsternberg jsternberg deleted the feat/experimental-preview branch May 10, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create experimental preview function for limiting the output

4 participants