[BUG][Discover] Check if the timestamp is already included to remove duplicate col #6983
Conversation
ananzh
requested review from
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi and
mengweieric
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6983 +/- ##
==========================================
+ Coverage 67.45% 67.46% +0.01%
==========================================
Files 3442 3443 +1
Lines 67816 67829 +13
Branches 11027 11035 +8
==========================================
+ Hits 45742 45761 +19
+ Misses 19408 19398 -10
- Partials 2666 2670 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -29,7 +29,7 @@ | |||
| */ | |||
|
|
|||
| import React, { useRef, useEffect } from 'react'; | |||
| import { DocViewRenderFn, DocViewRenderProps } from '../../../doc_views/doc_views_types'; | |||
| import { DocViewRenderFn, DocViewRenderProps } from '../../doc_views/doc_views_types'; | |||
There was a problem hiding this comment.
How did this build if this wasn't imported correctly?
There was a problem hiding this comment.
I do see a type error when I debug the issue
Cannot find module '../../../doc_views/doc_views_types' or its corresponding type declarations.ts(2307)
I believe this is related to an old discussion #1660, which highlights that the standard build scripts in the project do not fail on or show TypeScript errors. This has led to a situation where TypeScript compilation errors (like the one I encountered) do not cause the build to fail, allowing many such errors to creep into the project unnoticed.
There was a problem hiding this comment.
I believe we don't have the bandwidth to address this issue right now, but we should consider implementing a way to prevent new type errors from being added. This will help ensure the problem doesn't grow larger.
CC: @AMoo-Miki @ashwin-pc
There was a problem hiding this comment.
Yeah typescript error right now rely on the IDE or us manually running tsc and validating the diff. I have a work in progress PR to help catch this in the CI but its not complete yet. #6586
| return !hideTimeField && | ||
| indexPattern.timeFieldName && | ||
| !columns.includes(indexPattern.timeFieldName) |
There was a problem hiding this comment.
nit: consider assigning this to a meaningfully named variable so the reader doesn't have to parse out what this is trying to do
There was a problem hiding this comment.
Will think a good name and update
Issue Resolve opensearch-project#6982 Signed-off-by: Anan Zhuang <ananzh@amazon.com>
Signed-off-by: Anan Zhuang <ananzh@amazon.com>
…duplicate col (#6983) * check if the timestamp is already included to remove duplicate col --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 0db5848) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…duplicate col (opensearch-project#6983) * check if the timestamp is already included to remove duplicate col --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…duplicate col (#6983) * check if the timestamp is already included to remove duplicate col --------- Signed-off-by: Anan Zhuang <ananzh@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 0db5848) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…duplicate col (#6983) (#7399) * check if the timestamp is already included to remove duplicate col --------- (cherry picked from commit 0db5848) Signed-off-by: Anan Zhuang <ananzh@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…duplicate col (#6983) (#7047) * check if the timestamp is already included to remove duplicate col --------- (cherry picked from commit 0db5848) Signed-off-by: Anan Zhuang <ananzh@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Issues Resolved
#6982
Screenshot
Changelog
Check List
yarn test:jestyarn test:jest_integration