Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-13523][table-planner-blink] Verify and correct arithmetic function's semantic for Blink planner #9331

Closed
wants to merge 7 commits into from

Conversation

@docete
Copy link
Contributor

commented Aug 2, 2019

What is the purpose of the change

Verify and correct arithmetic function's semantic for Blink planner

Brief change log

  • Refactor arithmetic divide to keep it compatible with old planner
  • Refactor avg agg to keep it compatible with old planner
  • Remove non-standard bitwise/div scalar function from Blink planner

Verifying this change

This change is already covered by existing tests.

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

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

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
@flinkbot

This comment has been minimized.

Copy link

commented Aug 2, 2019

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 2c9256d (Fri Aug 23 10:14:57 UTC 2019)

Warnings:

  • No documentation files were touched! Remember to keep the Flink docs up to date!

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • 1. The [description] looks good.
  • 2. There is [consensus] that the contribution should go into to Flink.
  • 3. Needs [attention] from.
  • 4. The change fits into the overall [architecture].
  • 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier
@flinkbot

This comment has been minimized.

Copy link

commented Aug 2, 2019

CI report:

@wuchong

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

@JingsongLi , could you help to review this PR too ?

Copy link
Member

left a comment

Hi @docete , there are some cases failed in blink planner. Please have a look.

Here are some other suggestions:

  1. org.apache.flink.table.planner.functions.aggfunctions.IntegralAvgAggFunction is never used now, we can remove it.
  2. The AvgAggFunction under test package org.apache.flink.table.planner.utils can be removed too.
@docete docete force-pushed the docete:FLINK-13523 branch from 5e757e1 to 6069d51 Aug 6, 2019
@docete

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Hi @docete , there are some cases failed in blink planner. Please have a look.

Here are some other suggestions:

  1. org.apache.flink.table.planner.functions.aggfunctions.IntegralAvgAggFunction is never used now, we can remove it.
    OK, I will remove it.
  1. The AvgAggFunction under test package org.apache.flink.table.planner.utils can be removed too.
    This class is used by org.apache.flink.table.planner.runtime.batch.sql.agg.WindowAggregateITCase
@docete docete force-pushed the docete:FLINK-13523 branch 2 times, most recently from 49f315b to 33059bd Aug 6, 2019
@JingsongLi

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Maybe this PR will affect TPC-H result too, @docete can you verify it manually?

@docete docete force-pushed the docete:FLINK-13523 branch from 33059bd to 25b39b0 Aug 6, 2019
@docete

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Maybe this PR will affect TPC-H result too, @docete can you verify it manually?

OK, I will verify it manually.

@docete docete force-pushed the docete:FLINK-13523 branch from 25b39b0 to 48c66a9 Aug 7, 2019
@docete

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Hi @JingsongLi I verified tpch e2e test locally
[PASS] 'flink-end-to-end-tests/test-scripts/test_tpch.sh' passed after 2 minutes and 17 seconds! Test exited with exit code 0.

@docete

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Passed pre-commit test on my travis: https://travis-ci.com/docete/flink/builds/122209856

@wuchong

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

The TPC-H e2e tests are not affected because all the avg or / are on double fields.

Copy link
Member

left a comment

Thanks for the update @docete . I only left 2 minor comments.

@JingsongLi do you have other concerns?

@JingsongLi

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@docete Thanks update, left some comments. Especially for 1941bc6 , I think this commit need more discussion.

@docete docete force-pushed the docete:FLINK-13523 branch from 7d17148 to 8fefb13 Aug 8, 2019
@docete docete force-pushed the docete:FLINK-13523 branch from 8fefb13 to e674d47 Aug 8, 2019
@docete

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

rebase and resolve conflicts

@wuchong
wuchong approved these changes Aug 9, 2019
Copy link
Member

left a comment

LGTM.

@wuchong

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Merged with

4d560df
fbd3225
421f0a5

@wuchong wuchong closed this Aug 9, 2019
@docete docete deleted the docete:FLINK-13523 branch Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.