You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sp-202 (via GitHub)" <gi...@apache.org> on 2025/12/24 06:12:41 UTC
[PR] Feat/hive support [superset]
sp-202 opened a new pull request, #36821:
URL: https://github.com/apache/superset/pull/36821
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #36821:
URL: https://github.com/apache/superset/pull/36821#issuecomment-3725300103
@sp-202 Can you add a description for this? I believe we already support hive. Once you fill out the description, feel free to move it out of draft.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36821:
URL: https://github.com/apache/superset/pull/36821#issuecomment-4599015718
Closing this draft as it appears to be deployment-specific Docker/compose scaffolding (external 'databricks-net' network, debug logging) rather than a general Hive feature for upstream. Happy to reconsider a focused, generalized version if you'd like to propose one. Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36821:
URL: https://github.com/apache/superset/pull/36821#issuecomment-3688803543
## Nitpicks ๐
<table>
<tr><td>๐ <strong>No security issues identified</strong></td></tr>
<tr><td>โก <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/36821/files#diff-2fa9979d922d5b77f49fa43d7409876be00cf3d7cd6fca4e83ca02a6aa8861cbR19-R51'><strong>Sensitive logs</strong></a><br>The new exec redirection routes all stdout/stderr into /app/docker/debug.log. That will capture any secrets or sensitive environment values printed later (for example DB URIs, passwords, or exported test URIs), so reviewers should validate what information may be emitted and whether sensitive values are filtered or masked before being logged.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36821/files#diff-2fa9979d922d5b77f49fa43d7409876be00cf3d7cd6fca4e83ca02a6aa8861cbR19-R19'><strong>Log file management</strong></a><br>Appending all container output to a single file inside the container can cause uncontrolled file growth, lack of rotation, and permission issues. Confirm that the target directory exists, appropriate file permissions are set, and a rotation/retention strategy is in place to avoid disk exhaustion and information exposure.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36821/files#diff-18258246a98da2c9d9e3155b18707b7038f042615e8159a7ea4f5e9bfec1ada9R1-R1'><strong>Reproducibility / Pinning</strong></a><br>The base image is referenced as `apache/superset:latest` and the pip packages are installed without pinned versions.
Unpinned images and packages make builds non-reproducible and increase the chance of unexpected breakage or supply-chain vulnerabilities.
Consider pinning the base image and the Python packages to explicit versions or using a lockfile.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36821/files#diff-18258246a98da2c9d9e3155b18707b7038f042615e8159a7ea4f5e9bfec1ada9R6-R11'><strong>Image size / build deps left behind</strong></a><br>The Dockerfile installs build-time packages (e.g. `build-essential`, `libsasl2-dev`, `libpq-dev`) but does not remove them after building Python wheels.
This leaves unnecessary toolchains in the final image, increasing image size and attack surface. Use a single RUN layer that installs deps, installs Python packages, then uninstalls build-only deps (or use multi-stage build).<br>
- [ ] <a href='https://github.com/apache/superset/pull/36821/files#diff-18258246a98da2c9d9e3155b18707b7038f042615e8159a7ea4f5e9bfec1ada9R14-R14'><strong>Unclear pip / venv invocation</strong></a><br>The line uses `uv pip install --system --python /app/.venv ...`. It's not obvious whether `uv` is present in the base image and whether this reliably installs into the intended virtualenv.
Prefer invoking the venv's pip directly (`/app/.venv/bin/pip`) or using a known wrapper to avoid hidden dependencies/failures.<br>
</td></tr>
</table>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36821:
URL: https://github.com/apache/superset/pull/36821#issuecomment-3688806711
CodeAnt AI finished reviewing your PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #36821: Feat/hive support
URL: https://github.com/apache/superset/pull/36821
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #36821:
URL: https://github.com/apache/superset/pull/36821#discussion_r2644899051
##########
docker/docker-bootstrap.sh:
##########
@@ -42,7 +42,7 @@ if [ "$CYPRESS_CONFIG" == "true" ]; then
PORT=8081
fi
# Skip postgres requirements installation for workers to avoid conflicts
-if [[ "$DATABASE_DIALECT" == postgres* ]] && [ "$(whoami)" = "root" ] && [ "$1" != "worker" ] && [ "$1" != "beat" ]; then
+if [[ "$DATABASE_DIALECT" == postgres* ]] && [ "$(whoami)" = "root" ] && [ "$1" != "worker" ] && [ "$1" != "beat" ] && [ "$SUPERSET_ENV" != "production" ]; then
Review Comment:
**Suggestion:** Using `whoami` in a command substitution can fail on minimal images (or be unavailable); replace it with a numeric UID check (`id -u`) which is more robust and avoids depending on `whoami`. [possible bug]
**Severity Level:** Critical ๐จ
```suggestion
if [[ "$DATABASE_DIALECT" == postgres* ]] && [ "$(id -u)" -eq 0 ] && [ "$1" != "worker" ] && [ "$1" != "beat" ] && [ "$SUPERSET_ENV" != "production" ]; then
```
<details>
<summary><b>Why it matters? โญ </b></summary>
Replacing whoami with id -u -eq 0 is more robust in minimal containers and POSIX-y. It's a sensible hardening to avoid runtime failure when whoami isn't present. Be mindful to use the same approach consistently across the script.
</details>
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docker/docker-bootstrap.sh
**Line:** 45:45
**Comment:**
*Possible Bug: Using `whoami` in a command substitution can fail on minimal images (or be unavailable); replace it with a numeric UID check (`id -u`) which is more robust and avoids depending on `whoami`.
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>
##########
debug_hive.py:
##########
@@ -0,0 +1,44 @@
+import socket
+import sys
+
+host = 'hive-server'
+port = 10000
+
+print(f"Testing connectivity to {host}:{port}...")
+
+# Test 1: DNS Resolution
+try:
+ ip = socket.gethostbyname(host)
+ print(f"DNS Resolved: {host} -> {ip}")
+except Exception as e:
+ print(f"DNS Failed: {e}")
+ sys.exit(1)
+
+# Test 2: TCP Socket
+try:
+ s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+ s.settimeout(5)
+ s.connect((ip, port))
+ print(f"TCP Connection Successful to {ip}:{port}")
+ s.close()
Review Comment:
**Suggestion:** Resource leak: the TCP socket `s` is only closed on the success path; if `s.connect()` raises an exception the socket remains open. Use a context manager or ensure `s.close()` runs in a finally block so the socket is always closed. [resource leak]
**Severity Level:** Minor โ ๏ธ
```suggestion
# create_connection handles DNS/IPv4/IPv6 resolution and returns a socket that supports context manager
with socket.create_connection((ip, port), timeout=5) as s:
print(f"TCP Connection Successful to {ip}:{port}")
```
<details>
<summary><b>Why it matters? โญ </b></summary>
This is a valid resource-leak bug: if connect() raises, the socket object may be left open. The improved code uses socket.create_connection within a context manager which ensures the socket is closed on all paths and simplifies timeout handling. It's a straightforward correctness fix.
</details>
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** debug_hive.py
**Line:** 19:23
**Comment:**
*Resource Leak: Resource leak: the TCP socket `s` is only closed on the success path; if `s.connect()` raises an exception the socket remains open. Use a context manager or ensure `s.close()` runs in a finally block so the socket is always closed.
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>
##########
docker/Dockerfile.hive:
##########
@@ -0,0 +1,16 @@
+FROM apache/superset:latest
Review Comment:
**Suggestion:** Using the unpinned image tag `:latest` makes builds non-reproducible and can introduce breaking or insecure changes; expose a build argument so callers can pin the Superset base image/version. [possible bug]
**Severity Level:** Critical ๐จ
```suggestion
ARG SUPERSET_VERSION=latest
FROM apache/superset:${SUPERSET_VERSION}
```
<details>
<summary><b>Why it matters? โญ </b></summary>
Using :latest makes image builds non-reproducible and can silently pull breaking or insecure updates. Exposing a build ARG to allow pinning the Superset base image is a minimal, low-risk improvement that helps reproducible builds.
</details>
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docker/Dockerfile.hive
**Line:** 1:1
**Comment:**
*Possible Bug: Using the unpinned image tag `:latest` makes builds non-reproducible and can introduce breaking or insecure changes; expose a build argument so callers can pin the Superset base image/version.
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>
##########
docker/Dockerfile.hive:
##########
@@ -0,0 +1,16 @@
+FROM apache/superset:latest
+
+USER root
+
+# Install system dependencies for thrift-sasl
+RUN apt-get update -q \
+ && apt-get install -yq --no-install-recommends \
+ libsasl2-dev \
+ build-essential \
+ libpq-dev \
+ && rm -rf /var/lib/apt/lists/*
+
+# Install python dependencies inside the virtual environment using uv
+RUN uv pip install --system --python /app/.venv pyhive thrift thrift-sasl psycopg2-binary
Review Comment:
**Suggestion:** The command `uv pip ...` is invalid/unknown in the base image and will cause the RUN step to fail; also installing into a virtualenv path that may not exist will break the build. Use the venv's pip if present, otherwise fall back to the system pip, and add --no-cache-dir to avoid leaving pip cache in the layer. [possible bug]
**Severity Level:** Critical ๐จ
```suggestion
RUN if [ -x /app/.venv/bin/pip ]; then \
/app/.venv/bin/pip install --no-cache-dir pyhive thrift thrift-sasl psycopg2-binary; \
else \
python3 -m pip install --no-cache-dir pyhive thrift thrift-sasl psycopg2-binary; \
fi
```
<details>
<summary><b>Why it matters? โญ </b></summary>
The RUN line in the PR uses "uv pip ..." which looks wrong for the apache/superset image โ "uv" is not a standard wrapper and will likely make the build fail. The proposed fallback to use the venv pip if present or python3 -m pip is sensible and the addition of --no-cache-dir avoids leaving pip cache in the layer. This directly fixes a build-time break.
</details>
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docker/Dockerfile.hive
**Line:** 14:14
**Comment:**
*Possible Bug: The command `uv pip ...` is invalid/unknown in the base image and will cause the RUN step to fail; also installing into a virtualenv path that may not exist will break the build. Use the venv's pip if present, otherwise fall back to the system pip, and add --no-cache-dir to avoid leaving pip cache in the layer.
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>
##########
docker/docker-init.sh:
##########
@@ -16,6 +16,8 @@
# limitations under the License.
#
set -e
+exec > >(tee -a /app/docker/debug.log) 2>&1
Review Comment:
**Suggestion:** Robustness issue: if /app/docker doesn't exist or isn't writable, the process-substitution tee can fail or exit and break logging/output; check that the directory/file are creatable and writable and fall back to console-only logging when not writable. [possible bug]
**Severity Level:** Critical ๐จ
```suggestion
if mkdir -p /app/docker && touch /app/docker/debug.log && [ -w /app/docker/debug.log ]; then
exec > >(tee -a /app/docker/debug.log) 2>&1
else
echo "Warning: cannot write to /app/docker/debug.log, continuing with stdout/stderr only"
fi
```
<details>
<summary><b>Why it matters? โญ </b></summary>
The improved code safely checks that the directory/file exist and are writable and falls back to console logging if not โ that's a practical robustness improvement.
Note: in bash with set -e, commands inside the if condition won't abort the script, so this construct is safe; you may still want to combine this with permission setting or clear logging policy for mounted volumes.
</details>
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docker/docker-init.sh
**Line:** 19:19
**Comment:**
*Possible Bug: Robustness issue: if /app/docker doesn't exist or isn't writable, the process-substitution tee can fail or exit and break logging/output; check that the directory/file are creatable and writable and fall back to console-only logging when not writable.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36821:
URL: https://github.com/apache/superset/pull/36821#issuecomment-3688796471
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36821:
URL: https://github.com/apache/superset/pull/36821#discussion_r2644910793
##########
docker/Dockerfile.hive:
##########
@@ -0,0 +1,16 @@
+FROM apache/superset:latest
+
+USER root
+
+# Install system dependencies for thrift-sasl
+RUN apt-get update -q \
+ && apt-get install -yq --no-install-recommends \
+ libsasl2-dev \
+ build-essential \
+ libpq-dev \
+ && rm -rf /var/lib/apt/lists/*
+
+# Install python dependencies inside the virtual environment using uv
+RUN uv pip install --system --python /app/.venv pyhive thrift thrift-sasl psycopg2-binary
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect UV Install</b></div>
<div id="fix">
The --system flag installs packages globally, not in the venv. Remove it to match main Dockerfile patterns.
</div>
</div>
<small><i>Code Review Run #c576fd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Feat/hive support [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36821:
URL: https://github.com/apache/superset/pull/36821#issuecomment-3688796276
CodeAnt AI is reviewing your PR.
---
### Thanks for using CodeAnt! ๐
We're free for open-source projects. if you're enjoying it, help us grow by sharing.
[Share on X](https://twitter.com/intent/tweet?text=Just%20tried%20%40CodeAntAI%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A&url=https%3A//codeant.ai) ยท
[Reddit](https://www.reddit.com/submit?title=Check%20out%20CodeAnt%20for%20automated%20code%20review&text=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos.%20Worth%20checking%20out%3A%20https%3A//codeant.ai) ยท
[LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcodeant.ai&mini=true&title=Check%20out%20CodeAnt%20for%20automated%20code%20review&summary=Just%20tried%20CodeAnt%20for%20automated%20code%20review%20and%20I%27m%20impressed%21%20Free%20for%20open%20source%20with%20a%20free%20trial%20for%20private%20repos)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org