Skip to content

Conversation

@twalthr
Copy link
Contributor

@twalthr twalthr commented May 15, 2017

This PR adds support for multiple consecutive windows for the Table API. SQL requires additional group auxiliary functions. I will open a followup issue for that.

Copy link
Contributor

@fhueske fhueske left a comment

Choose a reason for hiding this comment

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

Hi @twalthr,

looks very good. Just had minor comments. +1 to merge otherwise.

override private[flink] def validateInput(): ValidationResult = {
child match {
case WindowReference(_, Some(tpe)) if !isRowtimeIndicatorType(tpe) =>
ValidationFailure("A proctime window can not guarantee a rowtime attribute.")
Copy link
Contributor

Choose a reason for hiding this comment

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

-> "A proctime window cannot provide a rowtime attribute"?

}

private[flink] def toNamedWindowProperty(name: String)(implicit relBuilder: RelBuilder)
def toNamedWindowProperty(name: String)
Copy link
Contributor

Choose a reason for hiding this comment

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

define in one line?

import org.apache.flink.table.utils.TableTestBase
import org.apache.flink.table.utils.TableTestUtil._
import org.junit.Test
import org.junit.{Ignore, Test}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore is unused

import org.apache.flink.types.Row
import org.junit.Assert._
import org.junit.Test
import org.junit.{Ignore, Test}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore is unused

@twalthr
Copy link
Contributor Author

twalthr commented May 15, 2017

Thanks for the review @fhueske. I will merge this...

@asfgit asfgit closed this in c86f46c May 15, 2017
asfgit pushed a commit that referenced this pull request May 15, 2017
hequn8128 pushed a commit to hequn8128/flink that referenced this pull request Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants