Fix columns duplication on MongoDB Query Runner #6640 #6641
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
There was a problem hiding this comment.
@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?
|
@guidopetri @konnectr Thanks for your review.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
|
e2e test still failed. I don't know main branch status... |
|
e2e test passed by merging main branch. |
|
We are having a similar issue right now. Can you guys let me know if I can help in anyways? |
|
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. |
|
@clearnote01 @guidopetri any news about this merge? Please we need it! |
|
@justinclift |
|
Sorry, something is wrong. I will check it. Please wait. |
|
I fixed the merge issue. All checks have passed. @justinclift |
|
@wlach Do you have time/interest to look over this one? 😄 |
|
@eradman Do you have time to look over this one? It seems a bit more urgent than some of the other PRs around, as people are emailing me directly about this one to ask. 😉 |
I don't have enough knowledge about MongoDB to provide useful input on this. Perhaps merge it and ask for feedback? |
|
Good idea, lets do that. 😄 |
|
@masayuki038 Thanks for taking the time to get this over the line. 😄 |
|
@justinclift @eradman Thanks for your reviews! I checked it on local, but let me know if someone reported some issues around this. I am checking github every day 😄 Thank you. |
|
Will do @masayuki038. 😄 |
|
@justinclift @masayuki038 it is working very well now! thank you very much guys! |
What type of PR is this?
Description
Bug fix for #6640.
And added some tests for MongoDB Query Runner.
How is this tested?
Related Tickets & Documents
Closes #6640
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
without
fieldswith
fields