Skip to content

fix: bar smapling not work bug. fix#4571 - #4573

Merged
xile611 merged 3 commits into
developfrom
fix/bug-for-sampling
Sep 11, 2026
Merged

xile611 merged 3 commits into
developfrom
fix/bug-for-sampling

Conversation

@skie1997

Copy link
Copy Markdown
Contributor

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Refactoring
  • Update dependency
  • Code style optimization
  • Test Case
  • Branch merge
  • Release
  • Site / documentation update
  • Demo update
  • Workflow
  • Other (about what?)

🔗 Related issue link

close #4571

🔗 Related PR link

🐞 Bugserver case id

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

🚀 Summary

copilot:summary

🔍 Walkthrough

copilot:walkthrough

@xuefei1313

Copy link
Copy Markdown
Contributor

🦞 Aime Bot Review

改动摘要:本 PR 针对 bar 系列在开启 sampling 时不生效的问题,将数据变换流水线中的 transform 类型从 sampling 调整为 dataSampling,并新增了一条带 sampling: 'lttb' 的 bar 单元测试和对应的 changelog 记录,用来验证修复效果。

代码层面的具体观察:在 BarSeries 中,对 _data 的 transform 配置改为 type: 'dataSampling',仍然沿用原有的 size / factor / yfield 参数,看起来是与数据集内部对采样 transform 的命名保持一致,从而让 sampling 配置真正生效。建议作者自查一下其他系列(例如 line/area)中是否也是使用 dataSampling 这个 transform 名,以确认命名统一,避免后续出现类似问题。新增的单元测试通过构造 5 条简单的 bar 数据并开启 sampling: 'lttb',在完整的 chart 生命周期后断言 _data.getProduct() 长度仍为 5,覆盖了“采样不应在 bar 上误删数据”的主路径。后续如果有堆叠/水平 bar 等更多组合场景,可以按需再补充测试用例,属于增强建议,不阻塞当前修复。

小提示:PR 标题以及 changelog 中的文案里 smapling 建议改成 sampling,这个属于小拼写问题,不影响功能,可以后续有机会顺带修正。

合并建议:整体改动范围很小、意图明确,且有针对性的单测覆盖。如果已经在本地验证过其他典型场景(例如不同方向或不同 samplingFactor),个人认为这个修复是 OK 的,可以作为 bugfix 合入。

@Issues-translate-bot

Copy link
Copy Markdown

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


🦞Aime Bot Review

Summary of changes: This PR aims at the problem that the bar series does not take effect when sampling is turned on. The transform type in the data transformation pipeline is adjusted from sampling to dataSampling, and a new bar unit test with sampling: 'lttb' and the corresponding changelog record are added to verify the repair effect.

Specific observations at the code level: In BarSeries, the transform configuration of _data is changed to type: 'dataSampling', and the original size / factor / yfield parameters are still used. It seems to be consistent with the naming of the sampling transform inside the data set, so that the sampling configuration can truly take effect. It is recommended that the author check whether the transform name dataSampling is also used in other series (such as line/area) to confirm that the naming is unified and avoid similar problems in the future. The new unit test constructs 5 simple bar data and turns on sampling: 'lttb', and asserts that the length of _data.getProduct() is still 5 after the complete chart life cycle, covering the main path of "sampling should not accidentally delete data on bar". If there are more combination scenarios such as stacking/horizontal bars in the future, you can add test cases as needed. This is an enhancement suggestion and does not block the current fix.

Tip: It is recommended to change smapling to sampling in the PR title and the copy in the changelog. This is a minor spelling problem and does not affect the functionality. It can be corrected later.

Merger suggestion: The overall scope of changes is small, the intention is clear, and targeted single test coverage. If other typical scenarios have been verified locally (such as different directions or different samplingFactor), I personally think this fix is ​​OK and can be incorporated as a bugfix.

@xuefei1313

Copy link
Copy Markdown
Contributor

@codex review

@xile611 xile611 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@skie1997 感谢修复!将 sampling 改为已注册的 dataSampling,能够解决开启采样后柱图不显示的问题,业务修复方向是正确的。

合并前麻烦处理以下两点:

  1. 修复新增测试的初始化方式,详见行内评论。将此提交应用到当前 develop(5d03e35856d093a2d6b0b4cd5bcf0a4a5dd7f9d8)后,运行柱图测试可复现释放阶段的 TypeError: this._compiler.removeEventListener is not a function。建议复用该文件已有的 createBarChart(samplingSpec)。
  2. 在 PR 描述中补充 BugServer case ID,并在同步 develop 后重跑失败的检查。PR 检查 明确提示:Can't find case id of bugserver in the pull request body;Bug Server CI 和 size 检查也仍处于失败状态,需要确认重跑结果。

我在临时副本中改用现有测试工厂后,柱图测试共 14 项通过,其中包含额外补充的 6 项大数据量采样验证,覆盖纵向、横向、分组及 samplingFactor。麻烦修复上述测试问题并确认 CI 通过后再合并,谢谢!

Comment thread packages/vchart/__tests__/unit/chart/bar.test.ts Outdated
@xile611
xile611 merged commit c07b139 into develop Sep 11, 2026
7 of 10 checks passed
@xile611
xile611 deleted the fix/bug-for-sampling branch September 11, 2026 09:50
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.

[Bug] 柱状图配置sampling之后,不显示

4 participants