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

Support the tck format for the output of EXPLAIN #265

Merged
merged 26 commits into from
Apr 3, 2023

Conversation

AntiTopQuark
Copy link
Contributor

@AntiTopQuark AntiTopQuark commented Mar 26, 2023

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

vesoft-inc/nebula#5274

Description:

Support the tck format for the output of EXPLAIN

How do you solve it?

In nebula-go,I have added a function called MakePlanByTck(), which is designed to return TCK-formatted result sets for the Nebula Console.

The MakePlanByTck() function will sequentially add columns for dependencies, profiling data, and empty operator info.

Since profiling data and operator info are also needed in the Row-formatted, the logic for generating profiling data and operator info have been separated into individual functions MakeProfilingData() and MakeOperatorInfo() for reusability.

Special notes for your reviewer, ex. impact of this fix, design document, etc:

related link:

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: -1.03 ⚠️

Comparison is base (94abfb9) 62.29% compared to head (83adce1) 61.26%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
- Coverage   62.29%   61.26%   -1.03%     
==========================================
  Files           9        9              
  Lines        2270     2308      +38     
==========================================
  Hits         1414     1414              
- Misses        711      749      +38     
  Partials      145      145              
Impacted Files Coverage Δ
result_set.go 59.09% <0.00%> (-2.48%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Sophie-Xie Sophie-Xie requested a review from Aiee March 29, 2023 10:36
@jievince jievince requested review from jievince and yixinglu March 30, 2023 04:19
Copy link
Contributor

@jievince jievince left a comment

Choose a reason for hiding this comment

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

Good job

Copy link
Contributor

@Aiee Aiee left a comment

Choose a reason for hiding this comment

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

Could you add some tests to this PR? You could choose a test case from the .feature files from the Nebula repo and add it here.

@AntiTopQuark
Copy link
Contributor Author

Could you add some tests to this PR? You could choose a test case from the .feature files from the Nebula repo and add it here.

Alright, I had considered adding some tests before. However, this feature only aims to specify the output format for explain/profile.

  • couldn't find any tck test cases for explain format = "dot". Tck tests have their own format and don't check the output format. I'm not quite sure how to write a test case for format = "tck".
  • Alternatively, should I write a unit test for MakePlanByTck()? Unit tests can only ensure the correctness of the function's output and can't test the output format of nebula-console.

@Aiee
Copy link
Contributor

Aiee commented Apr 3, 2023

Makes sense.

1 similar comment
@Aiee
Copy link
Contributor

Aiee commented Apr 3, 2023

Makes sense.

@Aiee Aiee merged commit bf9fe21 into vesoft-inc:master Apr 3, 2023
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.

4 participants