Skip to content

Conversation

amotl
Copy link
Member

@amotl amotl commented Sep 16, 2025

@amotl amotl added the refactoring Changing shape or layout, or moving content around. label Sep 16, 2025
Copy link

coderabbitai bot commented Sep 16, 2025

Warning

Rate limit exceeded

@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 49 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 2aef611 and 081eb03.

📒 Files selected for processing (4)
  • docs/integrate/index.md (1 hunks)
  • docs/integrate/r/index.md (1 hunks)
  • docs/integrate/r/tutorial.rst (8 hunks)
  • docs/topic/ml/index.md (1 hunks)

Walkthrough

Adds an R integration entry to the Integrations index, introduces a new R integration landing page, refactors the R tutorial structure and anchors, and updates the ML index to replace embedded R content with a seealso reference to the dedicated R page.

Changes

Cohort / File(s) Summary
Integrations navigation
docs/integrate/index.md
Inserted r/index into the integrations toctree between existing entries; no other edits.
R integration docs
docs/integrate/r/index.md, docs/integrate/r/tutorial.rst
Added a new R integration landing page with sections, grid card, and toctree; rewrote tutorial structure, headings, and anchor from cratedb-r to r-tutorial, with renamed sections and added Usage/Setup details.
ML topic index cross-link
docs/topic/ml/index.md
Removed duplicated R sections and replaced them with a {seealso} pointing to {ref}\r``; rest unchanged.

Sequence Diagram(s)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

new content

Suggested reviewers

  • karynzv
  • bmunkholm
  • hammerhead

Poem

A rabbit hops through docs so bright,
Leaves R-shaped prints in morning light.
A link here, a tutorial there—
Crossrefs tidied with gentle care.
With whiskered grin and speedy feet,
Integrations now feel neat. 🥕📘

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "R: Refactor ML tutorial to dedicated section" accurately and concisely reflects the primary change in the changeset (moving the R ML tutorial into a dedicated R integration section and updating ML index references), so it is directly related and clear for reviewers.
Description Check ✅ Passed The PR description is minimal but related to the changeset: it labels the work as refactoring and includes a preview URL and reference, so under the lenient criteria it is not off-topic and passes.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@amotl amotl marked this pull request as ready for review September 16, 2025 00:26
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
docs/integrate/r/tutorial.rst (2)

55-66: Prerequisites and install commands should include RPostgres (and DBI), not RPostgreSQL.

Users will otherwise install the wrong driver.

-- `R`_ (optionally with a third party tool like `RStudio`_)
-    - The `RPostgreSQL`_ library
-    - The `caret`_ library
+- `R`_ (optionally with a third‑party tool like `RStudio`_)
+    - The `RPostgres`_ driver (and `DBI`_)
+    - The `caret`_ library
@@
-    > install.packages("RPostgreSQL")
+    > install.packages("RPostgres")
+    > install.packages("DBI")
     > install.packages("caret")

300-357: Bug: inserting from the wrong data frame and using dbSendQuery without clearing results.

  • You enrich classified_dataset, but the INSERT loop reads from unclassified_dataset and [,5] (species) doesn’t exist there.
  • Prefer DBI::dbExecute() with parameters to avoid result handles and quoting issues.
-    > query <- "INSERT INTO iris (sepal_length, sepal_width, petal_length, petal_width, species) values ( %s, %s, %s, %s, '%s')"
-    > for (i in 1:(dim(unclassified_dataset)[1]) ) {
-    +     q <- sprintf(query,
-    +                  unclassified_dataset[i,1],
-    +                  unclassified_dataset[i,2],
-    +                  unclassified_dataset[i,3],
-    +                  unclassified_dataset[i,4],
-    +                  unclassified_dataset[i,5])
-    +     dbSendQuery(con, q)
-    + }
+    # Parameterized inserts to doc.iris from the enriched data
+    > sql <- "INSERT INTO iris (sepal_length, sepal_width, petal_length, petal_width, species)
+              VALUES ($1, $2, $3, $4, $5)"
+    > n <- nrow(classified_dataset)
+    > for (i in seq_len(n)) {
+    +   DBI::dbExecute(
+    +     con, sql,
+    +     params = list(classified_dataset[i, 1],
+    +                   classified_dataset[i, 2],
+    +                   classified_dataset[i, 3],
+    +                   classified_dataset[i, 4],
+    +                   classified_dataset[i, 5])
+    +   )
+    + }
🧹 Nitpick comments (3)
docs/integrate/r/index.md (2)

13-15: Use the correct platform name “macOS”.

Replace “MacOS” with “macOS” for correctness.

-It compiles and runs on a wide variety of UNIX platforms, Windows and MacOS.
+It compiles and runs on a wide variety of UNIX platforms, Windows and macOS.

4-8: Optional: avoid hot‑linking the R logo.

Consider vendoring the logo into the repo to prevent external dependency and flaky previews.

docs/integrate/r/tutorial.rst (1)

124-135: Optionally load RPostgres explicitly for clarity.

You already use namespaced RPostgres::Postgres(), but adding library(RPostgres) improves copy/paste experience.

 # Loading the DBI library.
 > library(DBI)
+> library(RPostgres)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 427d701 and 2aef611.

📒 Files selected for processing (4)
  • docs/integrate/index.md (1 hunks)
  • docs/integrate/r/index.md (1 hunks)
  • docs/integrate/r/tutorial.rst (8 hunks)
  • docs/topic/ml/index.md (1 hunks)
🔇 Additional comments (4)
docs/integrate/r/tutorial.rst (2)

1-2: Confirm intent to keep the legacy anchor.

Both labels exist (cratedb-r and r-tutorial). If the rename is intentional, keeping the old one preserves backlinks; otherwise, remove Line 1 to avoid duplicate canonical labels.


181-186: Claim without local figure: ensure the image exists.

Verify that /_assets/img/integrations/r-iris-features.png is present in the repo and included in the docs build.

docs/integrate/index.md (1)

63-64: LGTM: R added to Integrations TOC.

Navigation entry is correct and in the expected position.

docs/topic/ml/index.md (1)

202-204: LGTM: R content centralized via seealso.

The cross‑reference {ref} matches the new (r)= label.

@amotl amotl requested review from hammerhead and kneth September 16, 2025 00:37
@amotl amotl merged commit bddc9d0 into main Sep 16, 2025
2 of 3 checks passed
@amotl amotl deleted the r branch September 16, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Changing shape or layout, or moving content around.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant