Skip to content

[BEAM-2375] Rebase SQL DSL branch with master#3287

Closed
jbonofre wants to merge 24 commits intoapache:masterfrom
jbonofre:SQL_REBASE
Closed

[BEAM-2375] Rebase SQL DSL branch with master#3287
jbonofre wants to merge 24 commits intoapache:masterfrom
jbonofre:SQL_REBASE

Conversation

@jbonofre
Copy link
Member

@jbonofre jbonofre commented Jun 2, 2017

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify.
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

@jbonofre
Copy link
Member Author

jbonofre commented Jun 2, 2017

R: @xumingming
CC: @xumingmin

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.007%) to 70.685% when pulling ac39dbe on jbonofre:SQL_REBASE into 7a075cc on apache:master.

@xumingming
Copy link
Contributor

LGTM.

@xumingming
Copy link
Contributor

@jbonofre do you mind share the methods, I was thinking that this rebase might need two steps, because it if I rebase from master, it will conflict with DSL_SQL(as there are some breaking api changes to resolve), if i rebase again from DSL_SQL, it will conflict with master. So separate into two steps by first let DSL_SQL adopt the new breaking apis, then rebase from master.

@aaltay
Copy link
Member

aaltay commented Jun 5, 2017

cc: @davorbonaci

Hey @jbonofre, does this require a vote before merging a feature branch into master?

@davorbonaci
Copy link
Member

Not following this fully, but it seems like a merge from the SQL branch into master. I don't think we are ready for this just yet.

@xumingming's comment make sense to me. Let's merge master branch into the SQL branch, fix up there, and continue development as usual.

@mingmxu
Copy link

mingmxu commented Jun 5, 2017

+1 we're not ready to merge SQL branch into master, guess some miscommunication.

I think what @xumingming wants is pulling the latest code from master branch into SQL branch.

@xumingming
Copy link
Contributor

Yeah, my intention was to pull the latest code into DSL_SQL branch(sorry if there are miscommunication @jbonofre).

@jbonofre
Copy link
Member Author

jbonofre commented Jun 6, 2017

Dual rebase on master and DSL_SQL (to be up to date from both side).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 70.6% when pulling 7fffb9b on jbonofre:SQL_REBASE into 6d64c6e on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 70.607% when pulling 7fffb9b on jbonofre:SQL_REBASE into 6d64c6e on apache:master.

@davorbonaci
Copy link
Member

But we shouldn't be updating DSL_SQL branch -- that requires force push, and may lose data.

@jbonofre
Copy link
Member Author

jbonofre commented Jun 7, 2017

It's not a merge on master, it's a rebase of the DSL_SQL branch from master. So, we don't merge on master, we just update the DSL_SQL branch.

@jbonofre
Copy link
Member Author

jbonofre commented Jun 7, 2017

Let me fix the pull request (it should be base on DSL_SQL branch instead of master).

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 70.617% when pulling 4f6cb13 on jbonofre:SQL_REBASE into c189d5c on apache:master.

@xumingming
Copy link
Contributor

hi guys, can we boost this a little bit? This PR kinda blocks other several PRs.

@xumingming xumingming mentioned this pull request Jun 7, 2017
4 tasks
@jbonofre
Copy link
Member Author

jbonofre commented Jun 7, 2017

Yeah, let me clean it up. I'm on it.

jbonofre and others added 9 commits June 7, 2017 17:19
correct package from org.beam.dsls.sql to org.apache.beam.dsls.sql

update with checkstyle
Changes:
1. revert BEAM dependency to 0.6.0 to avoid impact of changes in master branch;
2. updates as discussion during review;

refine BeamSqlRowCoder
Add support for aggregation: global, HOP, TUMBLE, SESSION, only aggregation function COUNT

fix typo
@jbonofre
Copy link
Member Author

jbonofre commented Jun 7, 2017

Fixed by #3311

@jbonofre jbonofre closed this Jun 7, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.628% when pulling b47af03 on jbonofre:SQL_REBASE into 78b6e3c on apache:master.

@jbonofre jbonofre deleted the SQL_REBASE branch June 8, 2017 08:38
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.

7 participants