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

Fix columns duplication on MongoDB Query Runner #6640 #6641

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

masayuki038
Copy link
Collaborator

What type of PR is this?

  • Bug Fix

Description

Bug fix for #6640.
And added some tests for MongoDB Query Runner.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Closes #6640

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

without fields

image

with fields

image

Copy link

codecov bot commented Dec 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (939bec2) 63.42% compared to head (8ea665e) 63.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6641      +/-   ##
==========================================
+ Coverage   63.42%   63.72%   +0.29%     
==========================================
  Files         162      162              
  Lines       13173    13174       +1     
  Branches     1819     1820       +1     
==========================================
+ Hits         8355     8395      +40     
+ Misses       4522     4473      -49     
- Partials      296      306      +10     
Files Coverage Δ
redash/query_runner/mongodb.py 60.09% <100.00%> (+19.03%) ⬆️

konnectr
konnectr previously approved these changes Jan 27, 2024
Copy link
Collaborator

@guidopetri guidopetri left a comment

Choose a reason for hiding this comment

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

@masayuki038 thanks for this fix. It looks like two of the tests aren't passing for it? Could you take a look and investigate why?

@masayuki038
Copy link
Collaborator Author

@guidopetri @konnectr Thanks for your review.

It looks like two of the tests aren't passing for it? Could you take a look and investigate why?

Sure. I missed something. I'll check it later. Please give me a little time.

"type": TYPES_MAP.get(type(value), TYPE_STRING),
}
)
if _get_column_by_name(columns, column_name) is None:
Copy link

Choose a reason for hiding this comment

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

First of all thanks for the fix on this, I was having this problem recently : D

In here, to make this more efficient, can't we just get the first row, and then generate the columns array from that outside for this for loop?

In this case every cell in the table requires this comparison which is in-efficient and whether we check row1 or rowN the columns will be same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@clearnote01 I understand your comment. I also think that if it is normal RDB data, I should do that.

I thought that MongoDB can have a different column for each row, so I implemented it like this. Does such a case not exist?

Choose a reason for hiding this comment

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

@masayuki038 You are right, I don't think it's a problem when projection is specifically configured via $project, but there could be other cases that would break it.

Maybe it's safer to keep it as it is, and have a note that this could be optimized in the future if someone can rule out the edge cases.

@masayuki038
Copy link
Collaborator Author

e2e test still failed. I don't know main branch status...
I will re-run it after #6748 is fixed.

@masayuki038
Copy link
Collaborator Author

e2e test passed by merging main branch.
@guidopetri Could you check this if you have some time? Thank you always.

@MrSpark2591
Copy link

We are having a similar issue right now. Can you guys let me know if I can help in anyways?

@jagal17
Copy link

jagal17 commented May 6, 2024

Hi, any news about this? I have also the same problem all the columns of my table are duplicated since I upgraded to the last preview version.

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.

MongoDB Query Runner displays duplicated columns
6 participants