审查贡献#

原则#

Arrow 是一个基础项目，需要在未来多年甚至数十年内不断发展，同时服务于潜在的数百万用户。我们认为，在审查时一丝不苟，比宽容和追求快速合并给项目带来更大的回报。

这样的代码审查可以提高代码质量，让更多的人参与并理解被更改的代码，并使项目总体上更健康，有更多的空间来成长并适应新兴需求。

指南#

元信息#

运用您自己的判断。这些指南不是硬性规定。提交者应该对其工作领域有足够的专业知识，能够根据他们的任何顾虑调整方法。

这些指南没有特定的排列顺序，也不打算用作清单。

最后，这些指南并非详尽无遗。

范围和完整性#

我们的一般政策是不引入回归或合并需要后续操作才能正常工作的 PR（尽管可以对此进行例外处理）。在合并后进行必要的更改对贡献者和审查者来说成本更高，而且对于其他开发人员来说，如果他们遇到由合并的 PR 引入的问题，也会感到困惑。
PR 的范围是什么，以及哪些更改可能/可以/应该超出范围并创建一个后续问题，应由作者和审查者共同确定。
当贡献一个大型功能并且似乎希望分段集成时，在决定如何划分更改时，倾向于功能上的凝聚力（例如，如果贡献一个文件系统实现，则第一个 PR 可以贡献目录元数据操作，第二个 PR 文件读取工具，第三个 PR 文件写入工具）。

公共 API 设计#

公共 API 应该引导用户使用最理想的构造。换句话说，如果有一种“最佳”方式来做某事，那么它也应该是最容易被发现的，并且输入起来最简洁。例如，与可能崩溃或在无效输入上静默产生错误结果的不安全 API 相比，安全 API 应该更加突出。
公共 API 理想情况下应该倾向于生成可读的代码。一个例子是当预计随着时间的推移添加多个选项时：最好尝试在逻辑上组织选项，而不是将它们全部并列在函数的签名中（例如，请参见 C++ 和 Python 中的 CSV 读取 API）。
命名很重要。试着问问自己，调用新 API 的代码是否无需阅读 API 文档就能理解。应避免含糊的命名；不准确的命名甚至更糟，因为它会误导读者并导致用户代码出现错误。
注意术语。每个项目都有（明确或不言而喻的）关于如何命名重要概念的约定；偏离这些约定会增加贡献者和项目用户的认知负担。相反，将一个众所周知的术语用于与通常不同的目的也会增加认知负担，并使开发人员的生活更加困难。
如果您不确定某个 API 是否适合手头的任务，建议将其标记为实验性的，这样用户就知道它可能会随着时间的推移而更改，而贡献者则不太担心带来破坏代码的改进。但是，实验性 API 不应被用作逃避基本 API 设计原则的借口。

健壮性#

Arrow 是一组开源库，将在非常广泛的环境中使用（包括在 Jupyter 解释器提示符下处理故意的人工数据）。如果您正在编写公共 API，请确保它不会在不寻常（但有效）的输入上崩溃或产生未定义的行为。
当实现非平凡的算法时，防御性编码可能有助于捕获潜在问题（例如，如果该语言允许，则仅限调试的断言）。
摄取可能不受信任的数据的 API（例如磁盘上的文件格式）应尽量避免在向其提供无效或损坏的数据时崩溃或产生静默错误。这可能需要大量的注意，这超出了常规代码审查的范围（例如设置模糊测试），但仍然可以在代码审查阶段提出基本检查。
在调用外部 API 时，尤其是系统函数或处理输入/输出的 API，请检查错误并传播它们（如果该语言没有自动传播错误，例如 C++）。

性能#

考虑性能，但不要过于痴迷。如果输入大小可能“很大”（大的含义取决于上下文：使用您自己的专业知识来决定！），则算法复杂度很重要。在对性能敏感的功能上，微优化提高 20% 或更多的性能可能也很有用；较小的微优化可能不值得花费时间，尤其是当它们导致更复杂的代码时。
如果性能很重要，请对其进行测量。不要满足于猜测和直觉（这可能基于关于编译器或硬件的错误假设）。

另请参阅

基准测试 Arrow
尽量避免尝试通过以某种方式编写代码来欺骗编译器/解释器/运行时，除非它真的非常重要。这些技巧通常很脆弱，并且依赖于可能过时的平台细节，并且它们会使代码更难以维护（一个可能阻止贡献者的常见问题是“我应该如何处理我的更改想要删除的这个奇怪的黑客？”）。
避免粗糙的边缘或退化行为（例如，当大小估计不准确时，内存爆炸）可能比尝试通过少量改进常见情况更重要。

文档#

这些指南应该理想地适用于散文文档和代码中的文档字符串。

查找含糊/信息量不足的措辞。例如，“如果...发生错误” 不如 “如果...会引发错误” 或 “如果...行为未定义” 信息丰富（第一种措辞没有告诉读者在这种错误发生时实际上会发生什么）。
在审查文档更改（或一般散文片段）时，请注意拼写、语法、表达和简洁。清晰的沟通对于与来自各种背景的人进行有效协作至关重要，并有助于编写更好的文档。
一些贡献者的母语不是英语（也许您也不是）。建议帮助他们和/或在需要时寻求外部帮助。
交叉链接增加了文档的全局价值。特别是 Sphinx 具有强大的交叉链接功能（包括主题引用、词汇表术语、API 引用），因此请务必加以利用！

测试#

当添加 API 时，所有名义上的情况都应该有测试用例。函数允许空值吗？那么应该测试空值（当然，还有非空值）。函数允许不同的输入类型吗？等等。
如果功能的某些方面很微妙（无论是通过定义还是作为实现细节），都应该对其进行测试。
应该测试边界情况，尤其是在 C++ 等底层实现语言中。示例：空数组、零块数组、仅包含空值的数组等。
压力测试可能很有用，例如，如果在添加非平凡的并行化时发现同步错误，或者针对缓慢而直接的参考实现验证计算参数。
然而，一个需要减轻的顾虑是运行测试套件的总体成本。持续集成（CI）运行时间可能非常长，我们应该警惕过度增加它们。有时，微调测试参数以平衡测试的有用性和运行它们的成本是值得的（尤其是在涉及压力测试时，因为它们往往意味着对大型数据集的执行）。

标签#

在审查 PR 时，我们应该尝试确定是否需要使用以下一个或两个问题标签标记相应的问题

关键修复：该更改修复了以下任一项：（a）安全漏洞；（b）导致生成不正确或无效数据的错误；或（c）导致崩溃的错误（同时 API 约定得到维护）。这旨在标记对可能在用户不知情的情况下影响用户的问题的修复。因此，修复导致错误的错误不算数，因为这些错误通常很明显。导致崩溃的错误被认为是关键的，因为它们可能是拒绝服务攻击的载体。
重大变更：该更改破坏了公共 API 中的向后兼容性。对于 C++ 中的更改，这不包括简单地破坏 ABI 兼容性的更改，除了我们确实保证 ABI 兼容性的少数几个地方（例如 C 数据接口）。实验性 API 不免除此规则；它们只是更有可能与此标签相关联。

重大变更和关键修复是分开的：重大变更会更改 API 约定，而关键修复使实现与现有的 API 约定保持一致。例如，修复导致 Parquet 读取器跳过包含数字 42 的行的错误是一个关键修复，但不是一个重大变更，因为行跳过不是合理用户会依赖的行为。

这些标签用于发布版本中，以突出显示用户在考虑升级库版本时应该注意的更改。重大变更有助于识别用户可能希望等到他们有时间调整其代码时再升级的原因，而关键修复则突出显示了不升级的风险。

此外，我们使用以下标签来指示优先级

优先级：阻塞：表示更改必须在下次发布之前合并。这包括对测试或打包失败的修复，这些失败会阻止发布成功完成最终打包或验证。
优先级：关键：表示高优先级问题。这是标记为“关键修复”的问题的超集，因为它还包含对导致错误和崩溃的某些问题的修复。

协作者#

协作者角色允许向用户授予分类角色，以便能够帮助分类问题。该角色允许用户标记和分配问题。

用户可以要求成为协作者，或者当他们在项目中合作一段时间后，可以由另一个社区成员提名。协作可以但不限于：创建 PR、回答问题、创建问题、审查 PR 等等。

为了提名某人作为协作者，你必须创建一个 PR，将该用户添加到 .asf.yaml 上的协作者列表中。提交者可以审查用户的过往协作并批准。

一段时间不活跃的协作者可以从协作者列表中删除。