Skip to content

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented Jun 6, 2023

What changes were proposed in this pull request?

The pr aims to assign names to the error class LEGACY_ERROR_TEMP[2433-2437].

Why are the changes needed?

Improve the error framework.

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

Exists test cases updated.

Copy link
Member

Choose a reason for hiding this comment

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

Can you trigger this errors from user space? Could you add some tests, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxGekk I created test cases use SimpleAnalyzer.checkAnalysis directly.

Copy link
Member

Choose a reason for hiding this comment

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

I see but if you cannot trigger the error from user space, we should convert the error to an internal one.

Copy link
Member

Choose a reason for hiding this comment

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

@beliefer Could you try to trigger it by from SQL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@beliefer Could you try to trigger it by from SQL.

We can't trigger it by SQL. Let me convert it to internal error.

Copy link
Member

@MaxGekk MaxGekk Jun 19, 2023

Choose a reason for hiding this comment

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

cc @cloud-fan Just to double check that the errors are internal.

@beliefer
Copy link
Contributor Author

ping @MaxGekk

@beliefer
Copy link
Contributor Author

ping @MaxGekk The GA failure is unrelate to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Internal errors have their own error class, could you use checkError(), please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.

Copy link
Member

Choose a reason for hiding this comment

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

Please, use checkError

Copy link
Member

@MaxGekk MaxGekk Jun 19, 2023

Choose a reason for hiding this comment

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

cc @cloud-fan Just to double check that the errors are internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this really internal? does the analyzer split select explode(...), explode(...) ... into two Projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me have a try.

@MaxGekk
Copy link
Member

MaxGekk commented Jun 22, 2023

@beliefer Could you resolve conflicts, please.

@beliefer
Copy link
Contributor Author

ping @MaxGekk Rebased.

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

@cloud-fan Are you ok with the changes?

@MaxGekk
Copy link
Member

MaxGekk commented Jun 28, 2023

+1, LGTM. Merging to master.
Thank you, @beliefer and @cloud-fan for review.

@MaxGekk MaxGekk closed this in 1c8c47c Jun 28, 2023
@beliefer
Copy link
Contributor Author

@MaxGekk @cloud-fan Thank you for all.

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.

3 participants