Repository: ClickHouse/ClickHouse
File: src/Access/AccessRights.cpp (C++)

Code before:
        if (other.children)
        {
            for (const auto & other_child : *other.children)
            {
                Node this_child = tryGetLeaf(other_child.node_name, other_child.level, !other_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        if (children)
        {
            for (const auto & this_child : *children)
            {
                Node other_child = other.tryGetLeaf(this_child.node_name, this_child.level, !this_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        return true;
    }

    void makeUnion(const Node & other)
    {
        /// We need these tmp nodes because union/intersect operations use the `getLeaf` function, which can't be made const.
        /// Potentially, we can use tryGetLeaf, but it's very complicated to traverse both trees at the same time:
        ///
        /// Tree1:
        ///              f (SELECT)
        ///           /            \
        ///        ancy (SELECT)   oo (SELECT)
        ///          |                 |
        ///    "" (SELECT, INSERT)    "" (SELECT, INSERT)
        ///
        /// Tree2:
        ///          foo (SELECT)
        ///              |
        ///         "" (SELECT, INSERT)
        ///
        /// Here Tree2.getLeaf("f") will return a valid node, but Tree2.tryGetLeaf("f") will return nullptr

        Node result;
        Node rhs = other;
        makeUnionRec(result, rhs);
        children = std::move(result.children);
        flags |= other.flags;
        optimizeTree();
    }

    void makeIntersection(const Node & other)

Review comment (suggestion):
This implementation performs `tryGetLeaf()` lookups inside loops over children on both sides, and then recurses, which can lead to repeated linear scans (notably since `tryGetLeaf()` searches children via `find_if`). For large grants trees, this can become quadratic (or worse) in the number of nodes. If `contains()` is used in hot/admin paths (e.g., backup restore validation across many objects), consider reducing repeated lookups by iterating children in a way that avoids per-child linear search (e.g., caching an index for the current node’s children for the duration of `contains()`, or using a lookup-friendly structure where appropriate).

Code after:
        /// 2) For each child in `this`, find the matching node in `other` and verify containment.
        ///
        /// The reverse traversal (step 2) is needed to handle partial revokes correctly.
        /// Example: GRANT SELECT ON *.*, REVOKE SELECT ON foo.*
        ///
        ///   this:                   other:
        ///       root (SELECT)         root (SELECT)
        ///        |
        ///      "foo" (SELECT)
        ///        |
        ///        "" (leaf, USAGE)
        ///
        /// Step 1 alone would pass because `other` has no children to check against, but `this` does not contain `other` because
        /// `this` has revoked SELECT on "foo".

        if (!flags.contains(other.flags))
            return false;

        if (other.children)
        {
            for (const auto & other_child : *other.children)
            {
                Node this_child = tryGetLeaf(other_child.node_name, other_child.level, !other_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        if (children)
        {
            for (const auto & this_child : *children)
            {
                Node other_child = other.tryGetLeaf(this_child.node_name, this_child.level, !this_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        return true;
    }

    void makeUnion(const Node & other)
    {
        /// We need these tmp nodes because union/intersect operations use the `getLeaf` function, which can't be made const.
        /// Potentially, we can use tryGetLeaf, but it's very complicated to traverse both trees at the same time:
        ///
        /// Tree1:
        ///              f (SELECT)
        ///           /            \
        ///        ancy (SELECT)   oo (SELECT)
        ///          |                 |