Skip to content

Conversation

@maqingxiang
Copy link
Contributor

No description provided.

Copy link
Contributor

@yanghua yanghua left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @maqingxiang . LGTM

@fhueske
Copy link
Contributor

fhueske commented Dec 12, 2018

Hi @maqingxiang, thanks for this PR.
The current documentation is correct.
For intersect / intersectAll, both input tables only need to have the same field types but the field names can differ. I'd like to keep the docs as they are to make this behavior clear.
Would you please close this PR?

Thank you, Fabian.

@yanghua
Copy link
Contributor

yanghua commented Dec 13, 2018

@fhueske you are right.
But there may be some points that can be misleading for some users:

  • Sometimes fields of the same name are more easily visually identified as having the same type;
  • Multiple test methods in SetOperatorsITCase explicitly specify the same fields (a, b, c) for the left and right Tables, because the document is just an incomplete example, sometimes after the user views the document, they will try to search the source code and see real cases in the project;
  • Other operators of multi-table operations in the same document such as union, minus also use the same field name

This is why I said LGTM. But in fact, for those who understand the use of this operator, it does not matter whether it is modified or not.

@fhueske
Copy link
Contributor

fhueske commented Dec 13, 2018

Consistent documentation is a good point. However, I'd rather change the other examples than indicating a limitation that does not exist.

Regarding the tests, IMO the should not be considered as documentation.

@github-actions
Copy link

This PR is being marked as stale since it has not had any activity in the last 180 days.
If you would like to keep this PR alive, please leave a comment asking for a review.
If the PR has merge conflicts, update it with the latest from the base branch.

If you are having difficulty finding a reviewer, please reach out to the [community](https://flink.apache.org/what-is-flink/community/).

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 90 days, it will be automatically closed.

@flinkbot
Copy link
Collaborator

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@github-actions
Copy link

This PR has been closed since it has not had any activity in 120 days.
If you feel like this was a mistake, or you would like to continue working on it,
please feel free to re-open the PR and ask for a review.

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.

5 participants