codeant-ai-for-open-source[bot] commented on code in PR #36805: URL: https://github.com/apache/superset/pull/36805#discussion_r2642215931
########## docs/src/components/databases/DatabasePage.tsx: ########## @@ -0,0 +1,595 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React from 'react'; +import { + Card, + Collapse, + Table, + Tag, + Typography, + Alert, + Space, + Divider, + Tabs, +} from 'antd'; +import { + CheckCircleOutlined, + CloseCircleOutlined, + WarningOutlined, + LinkOutlined, + CodeOutlined, Review Comment: **Suggestion:** The `CodeOutlined` icon is imported but never used, which is dead code and may cause linting errors or confusion about intended but missing functionality. [code quality] **Severity Level:** Minor ⚠️ ```suggestion ``` <details> <summary><b>Why it matters? ⭐ </b></summary> CodeOutlined is imported but not used anywhere in the final file. Removing the unused import reduces dead code and avoids linter/compiler warnings. This is a trivial, correct cleanup. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/src/components/databases/DatabasePage.tsx **Line:** 37:37 **Comment:** *Code Quality: The `CodeOutlined` icon is imported but never used, which is dead code and may cause linting errors or confusion about intended but missing functionality. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## docs/src/components/databases/DatabasePage.tsx: ########## @@ -0,0 +1,595 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React from 'react'; +import { + Card, + Collapse, + Table, + Tag, + Typography, + Alert, + Space, + Divider, + Tabs, +} from 'antd'; +import { + CheckCircleOutlined, + CloseCircleOutlined, + WarningOutlined, + LinkOutlined, + CodeOutlined, + KeyOutlined, + SettingOutlined, + BookOutlined, +} from '@ant-design/icons'; +import type { DatabaseInfo } from './types'; + +// Simple code block component for connection strings +const CodeBlock: React.FC<{ children: React.ReactNode; language?: string }> = ({ + children, + language = 'text', +}) => ( + <pre + style={{ + background: 'var(--ifm-code-background)', + padding: '12px 16px', + borderRadius: '4px', + overflow: 'auto', + fontSize: '13px', + fontFamily: 'var(--ifm-font-family-monospace)', + }} + > + <code>{children}</code> Review Comment: **Suggestion:** The `language` prop passed to the `CodeBlock` component is currently ignored, so specifying a language like "bash" or "json" has no effect and prevents language-specific styling or syntax highlighting from ever working as intended. [logic error] **Severity Level:** Minor ⚠️ ```suggestion <code className={`language-${language}`}>{children}</code> ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The current CodeBlock accepts a language prop but never uses it, so adding a className like `language-<lang>` is a small, valid improvement to enable CSS-based syntax highlighting (Prism/Highlight.js, etc.). The suggested improved_code is correct and safe: it doesn't change runtime behavior other than adding a semantic class to the <code> element which is what most highlighter tools expect. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/src/components/databases/DatabasePage.tsx **Line:** 59:59 **Comment:** *Logic Error: The `language` prop passed to the `CodeBlock` component is currently ignored, so specifying a language like "bash" or "json" has no effect and prevents language-specific styling or syntax highlighting from ever working as intended. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## docs/src/components/databases/DatabaseIndex.tsx: ########## @@ -0,0 +1,429 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { useState, useMemo } from 'react'; +import { Card, Row, Col, Statistic, Table, Tag, Input, Select } from 'antd'; +import { + DatabaseOutlined, + CheckCircleOutlined, + ApiOutlined, + KeyOutlined, + SearchOutlined, + LinkOutlined, +} from '@ant-design/icons'; +import type { DatabaseData, DatabaseInfo, CompatibleDatabase } from './types'; + +interface DatabaseIndexProps { + data: DatabaseData; +} + +// Type for table entries (includes both regular DBs and compatible DBs) +interface TableEntry { + name: string; + category: string; + score: number; + max_score: number; + timeGrainCount: number; + hasDrivers: boolean; + hasAuthMethods: boolean; + hasConnectionString: boolean; + joins?: boolean; + subqueries?: boolean; + supports_dynamic_schema?: boolean; + supports_catalog?: boolean; + ssh_tunneling?: boolean; + documentation?: DatabaseInfo['documentation']; + // For compatible databases + isCompatible?: boolean; + compatibleWith?: string; + compatibleDescription?: string; +} + +// Category colors for visual distinction +const CATEGORY_COLORS: Record<string, string> = { + 'Cloud - AWS': 'orange', + 'Cloud - Google': 'blue', + 'Cloud - Azure': 'cyan', + 'Cloud Data Warehouses': 'purple', + 'Apache Projects': 'red', + 'Traditional RDBMS': 'green', + 'Analytical Databases': 'magenta', + 'Search & NoSQL': 'gold', + 'Query Engines': 'lime', + 'Other Databases': 'default', +}; + +// Get category for a database +function getCategory(name: string): string { + const nameLower = name.toLowerCase(); + + if (nameLower.includes('aws') || nameLower.includes('amazon')) + return 'Cloud - AWS'; + if (nameLower.includes('google') || nameLower.includes('bigquery')) + return 'Cloud - Google'; + if (nameLower.includes('azure') || nameLower.includes('microsoft')) + return 'Cloud - Azure'; + if (nameLower.includes('snowflake') || nameLower.includes('databricks')) + return 'Cloud Data Warehouses'; + if ( + nameLower.includes('apache') || + nameLower.includes('druid') || + nameLower.includes('hive') || + nameLower.includes('spark') + ) + return 'Apache Projects'; + if ( + nameLower.includes('postgres') || + nameLower.includes('mysql') || + nameLower.includes('sqlite') || + nameLower.includes('mariadb') + ) + return 'Traditional RDBMS'; + if ( + nameLower.includes('clickhouse') || + nameLower.includes('vertica') || + nameLower.includes('starrocks') + ) + return 'Analytical Databases'; + if ( + nameLower.includes('elastic') || + nameLower.includes('solr') || + nameLower.includes('couchbase') + ) + return 'Search & NoSQL'; + if (nameLower.includes('trino') || nameLower.includes('presto')) + return 'Query Engines'; + + return 'Other Databases'; +} + +// Count supported time grains +function countTimeGrains(db: DatabaseInfo): number { + if (!db.time_grains) return 0; + return Object.values(db.time_grains).filter(Boolean).length; +} + +const DatabaseIndex: React.FC<DatabaseIndexProps> = ({ data }) => { + const [searchText, setSearchText] = useState(''); + const [categoryFilter, setCategoryFilter] = useState<string | null>(null); + + const { statistics, databases } = data; + + // Convert databases object to array, including compatible databases + const databaseList = useMemo(() => { + const entries: TableEntry[] = []; + + Object.entries(databases).forEach(([name, db]) => { + // Add the main database + entries.push({ + ...db, + name, + category: getCategory(name), + timeGrainCount: countTimeGrains(db), + hasDrivers: (db.documentation?.drivers?.length ?? 0) > 0, + hasAuthMethods: (db.documentation?.authentication_methods?.length ?? 0) > 0, + hasConnectionString: Boolean( + db.documentation?.connection_string || + (db.documentation?.drivers?.length ?? 0) > 0 + ), + isCompatible: false, + }); + + // Add compatible databases from this database's documentation + const compatibleDbs = db.documentation?.compatible_databases ?? []; + compatibleDbs.forEach((compat) => { + // Check if this compatible DB already exists as a main entry + const existsAsMain = Object.keys(databases).some( + (dbName) => dbName.toLowerCase() === compat.name.toLowerCase() + ); + + if (!existsAsMain) { + entries.push({ + name: compat.name, + category: getCategory(compat.name), + // Compatible DBs inherit scores from parent + score: db.score, + max_score: db.max_score, + timeGrainCount: countTimeGrains(db), + hasDrivers: false, + hasAuthMethods: false, + hasConnectionString: Boolean(compat.connection_string), + joins: db.joins, + subqueries: db.subqueries, + supports_dynamic_schema: db.supports_dynamic_schema, + supports_catalog: db.supports_catalog, + ssh_tunneling: db.ssh_tunneling, + documentation: { + description: compat.description, + connection_string: compat.connection_string, + pypi_packages: compat.pypi_packages, + }, + isCompatible: true, + compatibleWith: name, + compatibleDescription: `Uses ${name} driver`, + }); + } + }); + }); + + return entries; + }, [databases]); + + // Filter and sort databases + const filteredDatabases = useMemo(() => { + return databaseList + .filter((db) => { + const matchesSearch = + !searchText || + db.name.toLowerCase().includes(searchText.toLowerCase()) || + db.documentation?.description + ?.toLowerCase() + .includes(searchText.toLowerCase()); + const matchesCategory = !categoryFilter || db.category === categoryFilter; + return matchesSearch && matchesCategory; + }) + .sort((a, b) => b.score - a.score); + }, [databaseList, searchText, categoryFilter]); + + // Get unique categories and counts for filter + const { categories, categoryCounts } = useMemo(() => { + const counts: Record<string, number> = {}; + databaseList.forEach((db) => { + counts[db.category] = (counts[db.category] || 0) + 1; + }); + return { + categories: Object.keys(counts).sort(), + categoryCounts: counts, + }; + }, [databaseList]); + + // Table columns + const columns = [ + { + title: 'Database', + dataIndex: 'name', + key: 'name', + sorter: (a: TableEntry, b: TableEntry) => a.name.localeCompare(b.name), + render: (name: string, record: TableEntry) => { + // Convert name to URL slug + const toSlug = (n: string) => n.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-|-$/g, ''); + + // Link to parent for compatible DBs, otherwise to own page + const linkTarget = record.isCompatible && record.compatibleWith + ? `/docs/databases/${toSlug(record.compatibleWith)}` + : `/docs/databases/${toSlug(name)}`; + + return ( + <div> + <a href={linkTarget}> + <strong>{name}</strong> + </a> + {record.isCompatible && record.compatibleWith && ( + <Tag + icon={<LinkOutlined />} + color="geekblue" + style={{ marginLeft: 8, fontSize: '11px' }} + > + {record.compatibleWith} compatible + </Tag> + )} + <div style={{ fontSize: '12px', color: '#666' }}> + {record.documentation?.description?.slice(0, 80)} + {(record.documentation?.description?.length ?? 0) > 80 ? '...' : ''} + </div> + </div> + ); + }, + }, + { + title: 'Category', + dataIndex: 'category', + key: 'category', + width: 160, + filters: categories.map((cat) => ({ text: cat, value: cat })), + onFilter: (value: React.Key | boolean, record: TableEntry) => + record.category === value, + render: (category: string) => ( + <Tag color={CATEGORY_COLORS[category] || 'default'}>{category}</Tag> + ), + }, + { + title: 'Score', + dataIndex: 'score', + key: 'score', + width: 80, + sorter: (a: TableEntry, b: TableEntry) => a.score - b.score, + defaultSortOrder: 'descend' as const, + render: (score: number, record: TableEntry) => ( + <span + style={{ + color: score > 150 ? '#52c41a' : score > 100 ? '#1890ff' : '#666', + fontWeight: score > 150 ? 'bold' : 'normal', + }} + > + {score}/{record.max_score} + </span> + ), + }, + { + title: 'Time Grains', + dataIndex: 'timeGrainCount', + key: 'timeGrainCount', + width: 100, + sorter: (a: TableEntry, b: TableEntry) => a.timeGrainCount - b.timeGrainCount, + render: (count: number) => ( + <span>{count > 0 ? `${count} grains` : '-'}</span> + ), + }, + { + title: 'Features', + key: 'features', + width: 200, + render: (_: unknown, record: TableEntry) => ( + <div style={{ display: 'flex', gap: '4px', flexWrap: 'wrap' }}> + {record.joins && <Tag color="green">JOINs</Tag>} + {record.subqueries && <Tag color="green">Subqueries</Tag>} + {record.supports_dynamic_schema && <Tag color="blue">Dynamic Schema</Tag>} + {record.supports_catalog && <Tag color="purple">Catalog</Tag>} + {record.ssh_tunneling && <Tag color="cyan">SSH</Tag>} + </div> + ), + }, + { + title: 'Documentation', + key: 'docs', + width: 150, + render: (_: unknown, record: TableEntry) => ( + <div style={{ display: 'flex', gap: '4px', flexWrap: 'wrap' }}> + {record.hasConnectionString && ( + <Tag icon={<ApiOutlined />} color="default"> + Connection + </Tag> + )} + {record.hasDrivers && ( + <Tag icon={<DatabaseOutlined />} color="default"> + Drivers + </Tag> + )} + {record.hasAuthMethods && ( + <Tag icon={<KeyOutlined />} color="default"> + Auth + </Tag> + )} + </div> + ), + }, + ]; + + return ( + <div className="database-index"> + {/* Statistics Cards */} + <Row gutter={[16, 16]} style={{ marginBottom: 24 }}> + <Col xs={12} sm={6}> + <Card> + <Statistic + title="Total Databases" + value={statistics.totalDatabases} + prefix={<DatabaseOutlined />} + /> + </Card> + </Col> + <Col xs={12} sm={6}> + <Card> + <Statistic + title="With Documentation" + value={statistics.withDocumentation} + prefix={<CheckCircleOutlined />} + suffix={`/ ${statistics.totalDatabases}`} + /> + </Card> + </Col> + <Col xs={12} sm={6}> + <Card> + <Statistic + title="Multiple Drivers" + value={statistics.withDrivers} + prefix={<ApiOutlined />} + /> + </Card> + </Col> + <Col xs={12} sm={6}> + <Card> + <Statistic + title="Auth Methods" + value={statistics.withAuthMethods} + prefix={<KeyOutlined />} + /> + </Card> + </Col> + </Row> + + {/* Filters */} + <Row gutter={[16, 16]} style={{ marginBottom: 16 }}> + <Col xs={24} sm={12}> + <Input + placeholder="Search databases..." + prefix={<SearchOutlined />} + value={searchText} + onChange={(e) => setSearchText(e.target.value)} + allowClear + /> + </Col> + <Col xs={24} sm={12}> + <Select + placeholder="Filter by category" + style={{ width: '100%' }} + value={categoryFilter} + onChange={setCategoryFilter} + allowClear + options={categories.map((cat) => ({ + label: ( + <span> + <Tag + color={CATEGORY_COLORS[cat] || 'default'} + style={{ marginRight: 8 }} + > + {categoryCounts[cat] || 0} + </Tag> + {cat} + </span> + ), + value: cat, + }))} + /> + </Col> + </Row> + + {/* Database Table */} + <Table + dataSource={filteredDatabases} + columns={columns} + rowKey="name" Review Comment: **Suggestion:** The table currently uses only the database name as the `rowKey`, but compatible databases can share the same name across multiple parent engines, leading to duplicate React keys (e.g., the same compatible database listed for multiple engines) which can cause incorrect row rendering, state leakage between rows, and hard-to-debug UI inconsistencies. A safer approach is to derive a unique key that incorporates both the database name and its parent when the entry represents a compatible database. [logic error] **Severity Level:** Minor ⚠️ ```suggestion rowKey={(record) => record.isCompatible && record.compatibleWith ? `${record.compatibleWith}::${record.name}` : record.name } ``` <details> <summary><b>Why it matters? ⭐ </b></summary> The PR's code builds compatible DB entries that set name to the compatible DB's name and compatibleWith to the parent. There's no deduplication across different parents (existsAsMain only checks main DBs), so two different parents could add the same compatible DB name as separate entries. Using the plain name as rowKey can produce duplicate React keys, which leads to rendering issues and state leakage. The proposed key combining compatibleWith and name is a simple, correct fix that guarantees uniqueness for compatible entries without changing business logic. </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** docs/src/components/databases/DatabaseIndex.tsx **Line:** 417:417 **Comment:** *Logic Error: The table currently uses only the database name as the `rowKey`, but compatible databases can share the same name across multiple parent engines, leading to duplicate React keys (e.g., the same compatible database listed for multiple engines) which can cause incorrect row rendering, state leakage between rows, and hard-to-debug UI inconsistencies. A safer approach is to derive a unique key that incorporates both the database name and its parent when the entry represents a compatible database. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
