Skip to content

SPARK-1109 wrong API docs for pyspark map function - #73

Closed
ScrapCodes wants to merge 1 commit into
apache:masterfrom
ScrapCodes:SPARK-1109/wrong-API-docs
Closed

SPARK-1109 wrong API docs for pyspark map function#73
ScrapCodes wants to merge 1 commit into
apache:masterfrom
ScrapCodes:SPARK-1109/wrong-API-docs

Conversation

@ScrapCodes

Copy link
Copy Markdown
Member

No description provided.

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

koertkuipers pushed a commit to tresata-opensource/spark that referenced this pull request Mar 4, 2014
Approximate distinct count
Added countApproxDistinct() to RDD and countApproxDistinctByKey() to PairRDDFunctions to approximately count distinct number of elements and distinct number of values per key, respectively. Both functions use HyperLogLog from stream-lib for counting. Both functions take a parameter that controls the trade-off between accuracy and memory consumption. Also added Scala docs and test suites for both methods.
@AmplabJenkins

Copy link
Copy Markdown

Merged build finished.

@AmplabJenkins

Copy link
Copy Markdown

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/12992/

@mateiz

Copy link
Copy Markdown
Contributor

Thanks, merged in 0.9 and master

asfgit pushed a commit that referenced this pull request Mar 5, 2014
Author: Prashant Sharma <prashant.s@imaginea.com>
Closes#73 from ScrapCodes/SPARK-1109/wrong-API-docs and squashes the following commits:
1a55b58 [Prashant Sharma] SPARK-1109 wrong API docs for pyspark map function
(cherry picked from commit 0283665)
Signed-off-by: Matei Zaharia <matei@databricks.com>
@asfgitasfgit closed this in 0283665Mar 5, 2014
@ScrapCodes
ScrapCodes deleted the SPARK-1109/wrong-API-docs branch June 3, 2015 06:00
clockfly added a commit to clockfly/spark that referenced this pull request Sep 22, 2016
…which supports partial aggregation.
This is cherry-pick of feature on open source master branch (hash: https://github.com/databricks/spark/commit/f003e0cf2dc4cd23fe619e9a24208cd502680d6a).
## What changes were proposed in this pull request?
This PR implements aggregation function `percentile_approx`. Function `percentile_approx` returns the approximate percentile(s) of a column at the given percentage(s). A percentile is a watermark value below which a given percentage of the column values fall. For example, the percentile of column `col` at percentage 50% is the median value of column `col`.
### Syntax:
```
# Returns percentile at a given percentage value. The approximation error can be reduced by increasing parameter accuracy, at the cost of memory.
percentile_approx(col, percentage [, accuracy])
# Returns percentile value array at given percentage value array
percentile_approx(col, array(percentage1 [, percentage2]...) [, accuracy])
```
### Features:
1. This function supports partial aggregation.
2. The memory consumption is bounded. The larger `accuracy` parameter we choose, we smaller error we get. The default accuracy value is 10000, to match with Hive default setting. Choose a smaller value for smaller memory footprint.
3. This function supports window function aggregation.
### Example usages:
```
## Returns the 25th percentile value, with default accuracy
SELECT percentile_approx(col, 0.25) FROM table
## Returns an array of percentile value (25th, 50th, 75th), with default accuracy
SELECT percentile_approx(col, array(0.25, 0.5, 0.75)) FROM table
## Returns 25th percentile value, with custom accuracy value 100, larger accuracy parameter yields smaller approximation error
SELECT percentile_approx(col, 0.25, 100) FROM table
## Returns the 25th, and 50th percentile values, with custom accuracy value 100
SELECT percentile_approx(col, array(0.25, 0.5), 100) FROM table
```
### NOTE:
1. The `percentile_approx` implementation is different from Hive, so the result returned on same query maybe slightly different with Hive. This implementation uses `QuantileSummaries` as the underlying probabilistic data structure, and mainly follows paper `Space-efficient Online Computation of Quantile Summaries` by Greenwald, Michael and Khanna, Sanjeev. (http://dx.doi.org/10.1145/375663.375670)`
2. The current implementation of `QuantileSummaries` doesn't support automatic compression. This PR has a rule to do compression automatically at the caller side, but it may not be optimal.
## How was this patch tested?
Unit test, and Sql query test.
## Acknowledgement
1. This PR's work in based on lw-lin's PR apache#14298, with improvements like supporting partial aggregation, fixing out of memory issue.
Author: Sean Zhong <seanzhongdatabricks.com>
Closesapache#14868 from clockfly/appro_percentile_try_2.
Author: Sean Zhong <seanzhong@databricks.com>
Closesapache#73 from clockfly/appro_percentile_branch_2.0.
robert3005 added a commit to robert3005/spark that referenced this pull request Jan 12, 2017
jlopezmalla pushed a commit to jlopezmalla/spark that referenced this pull request Nov 3, 2017
jamesrgrinter pushed a commit to jamesrgrinter/spark that referenced this pull request Apr 22, 2018
* Added Python API for mapr-streaming (kafka 0.9)
Signed-off-by: Rostyslav Sotnychenko <rsotnychenko@maprtech.com>
(cherry picked from commit c7de39f)
bzhaoopenstack pushed a commit to bzhaoopenstack/spark that referenced this pull request Sep 11, 2019
…t instead (apache#73)
Novaclient's add_floating_ip function is deprecated use neutron client instead
hn5092 added a commit to hn5092/spark that referenced this pull request Nov 21, 2019
hn5092 pushed a commit to hn5092/spark that referenced this pull request Nov 29, 2019
hn5092 added a commit to hn5092/spark that referenced this pull request Nov 29, 2019
yuexingri pushed a commit to yuexingri/spark that referenced this pull request Dec 9, 2019
yuexingri pushed a commit to yuexingri/spark that referenced this pull request Dec 9, 2019
arjunshroff pushed a commit to arjunshroff/spark that referenced this pull request Nov 24, 2020
MaxGekk added a commit to MaxGekk/spark that referenced this pull request Sep 2, 2026
… budget (apache#74)
### What changes were proposed in this pull request?
A plan only, no code: section 10 of `PLAN_TASK_32.md`, plus a pointer from `PLAN_MILESTONE_4.md`'s task-32 row and section 2.9.
Step B2 is the grouping change that turns step B1's fragment sharing into a hot-loop win. Today every calendar output still gets its own loop method (`CHRONO_WEIGHT` 50 against `GROUP_BUDGET` 16), so the shared civil-from-days prefix only ever fires in the epilogue. The gate measured B2's shape at 1.29x/1.57x/1.80x for two/three/four fields at AVX-512 and nearly the same at 128-bit (`PLAN_TASK_32.md` 7.2, 7.4), and measured the compile cliff away (7.5) - but it did so by forcing `groupBudget` to 200, which is the measurement rig, not a shippable rule: a wide budget also merges plain chains, and task 17 measured that losing (4436 vs 3150 M rows/s in the committed parity file).
The plan replaces that with a rule. `addOps` counts a fragment's prefix once per group and reports what an output saved by reusing one; `groupOutputs` gains a second join clause that admits an output past `GROUP_BUDGET` only when `saved > 0`, bounded by a new `fusedCeiling` emit option. That is section 3.2's original sketch with one correction: 3.2 wrote the test as `marginal <= budget`, which would have re-merged task 17's case (marginal 6); `saved > 0` keeps it split. Section 10.2 works the rule through the shapes this task has measured, including one recorded limitation (greedy output order can leave `month(d)` in a group without `year(d)`; correct, no worse than today, parked in the debt register).
The rest: weights re-counted from emitted instructions because they now bound a method (10.3); a ladder past four fields - 6, 8, 12 outputs over one date via `add_months(d, k)` literals - that sets `fusedCeiling` on throughput and `-XX:+PrintCompilation` time at both widths (10.4); which tests flip and which guard is added (10.5, including a byte-identity check over non-calendar shapes so clause 2 cannot leak past fragments); six registered predictions, one of them for task 45 to score (10.6); risks (10.7); a four-commit sequence after apache#73 and apache#64 have landed (10.8); and what stays out (10.9).
### Why are the changes needed?
The gate for B2 has cleared at both widths and the remaining work is a design choice with a measured wrong answer available (the wide budget). Writing the rule down before building it is the project's standing practice, and this one has a specific trap - 3.2's own sketch re-merges task 17's case - that is cheaper to catch on paper than in a parity regeneration diff.
### Does this PR introduce _any_ user-facing change?
No. Documentation only.
### How was this patch tested?
Not applicable - plan files only. ASCII checked; the prose width matches the file's existing convention.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Fable 5.1)
MaxGekk added a commit to MaxGekk/spark that referenced this pull request Sep 2, 2026
### What changes were proposed in this pull request?
Removes the runtime range check ("guard") that every calendar extraction (`year`, `month`, `dayofmonth`, `quarter`, `dayofyear`, `last_day`, `add_months`) has carried since task 26. The check declined a whole batch to the row engine whenever a day fell outside the narrowed civil-from-days range the fast lowering is exact over.
`hasChrono` (whose only job was deciding whether to allocate a guard accumulator) is deleted, and `emitEra` no longer emits the two compares or the mask ANDing/ORing that fed the accumulator - what's left is just the day-of-era arithmetic. The kernel ABI (`int run`, `STATUS_CHRONO_RANGE`), the status-return logic, and the evaluator's fallback routing and metrics are all left in place, unset, because the very next task needs them back.
Two tests that asserted the old decline-and-fall-back-to-the-row-engine behavior end to end were removed rather than rewritten to assert a weaker one - a test that documents "this can now silently return a different answer" as permanently green felt worse than no test. Two unit tests were rewritten to assert the new behavior (an out-of-range day is computed, not declined).
`PLAN_TASK_51.md` has the full record, including a section stating the regression this opens plainly. `PLAN_TASK_52.md` is the plan for closing it: a guard moved to the nodes that can actually manufacture an out-of-range day from unbounded runtime arithmetic (`date_add`/`date_sub` with a column offset), rather than one repeated at every calendar extraction downstream of a value whose range is usually already established by the time it gets there (a bare column, or a value CSE/fragment sharing has already checked once for a sibling field).
### Why are the changes needed?
The removed guard re-verified the same fact - "is this day in range" - at every calendar extraction reading a given value, even when that value's range was already established once, upstream, for every sibling field reading it. The one case where a fresh check is genuinely needed - a value manufactured by unbounded runtime arithmetic, not a bare column - is not most calendar extractions, so paying the check at every extraction is redundant on the common path. The fix is to move the check to where a new fact can actually be introduced, not to remove it outright; this PR does the removal half, tracked to be paired with task 52's replacement.
### Does this PR introduce _any_ user-facing change?
Yes. Before this change, a date pushed outside roughly [-12800, 33134] CE by `date_add`/`date_sub` and then read by a calendar function (`year`, `month`, etc.) would fall back to the row engine and return the row engine's correct answer. After this change, the same query runs on Varka's kernel and can silently return an incorrect calendar field instead. This is a deliberate, temporary trade recorded in `PLAN_TASK_51.md` section 4, closed by task 52.
### How was this patch tested?
Full Varka suite green at both vector widths (`-XX:MaxVectorSize=16` and default) in both `catalyst` and `sql` modules. `dev/lint-java`, `dev/scalastyle`, and `build/sbt catalyst/doc` all clean. No pinned fixture (`VarkaLoopEmitterSuite`'s line map, `VarkaShapeCacheSuite`'s shape hash) moved, and no committed benchmark number is expected to change, since the arithmetic an in-range day runs is unchanged.
#### Update: merged forward onto master now that apache#72 (task 32 step B1) has landed
PR apache#72 added its own shared-prefix fragment mechanism and, with it, a test asserting the removed guard still fired through that shared path. Merging it in surfaced two things:
- The guard's removal reaches the shared path for free: `emitChronoPrefixOnce` (PR apache#72's fragment-sharing entry point) calls the same `emitEra` this PR already stripped the guard from, so there was never a second copy to find and delete. The one test that needed changing - `"the range guard is shared with the prefix, and still declines the batch"` - now asserts the opposite and exists to catch a future regression if the shared path ever grows its own inlined guard instead of routing through `emitEra`.
- Removing the guard's bytecode shrinks every calendar node's tail, shared or not, which moves two numbers PR apache#72 had just committed: the epilogue's `HugeMethodLimit` crossing goes from 17 outputs to 40 (task 32 step B1) to **19 outputs to 44** (this PR). Re-measured directly with the same harness; `PLAN_TASK_32.md` section 7.1 keeps the original ladder alongside the new one, and `PLAN_TASK_51.md` section 4.1 has the detail.
Full suite green at both vector widths in both modules after the merge; `dev/lint-java`, `dev/scalastyle` and `build/sbt catalyst/doc` all clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Sonnet 5)
MaxGekk added a commit to MaxGekk/spark that referenced this pull request Sep 2, 2026
…nly what it cannot bound (apache#76)
### What changes were proposed in this pull request?
A plan only, no code. **Stacked on apache#73** (`PLAN_TASK_52.md` was created there), so until apache#73 merges this PR's diff also shows apache#73's commits; the change this PR adds is the last commit, `60f2dbc839f`.
The first `PLAN_TASK_52.md`, written in the same PR as task 51's guard removal, guarded only `date_add`/`date_sub` with a *column* offset, on the claim that a literal offset's magnitude "is visible to the compiler". Visible, yes; bounded, no: `DateVarkaSupport.foldDaysOffset` accepts any `Int` and nothing checks it, so `year(date_add(d, 20000000))` - the removed differential's own query - fuses on master today and, after task 51, returns a wrong year. The first plan would have left that in place.
The second version opens with the rule that produces the right scope. The narrowed range has a computable slack around the `[0001, 9999]` column contract (`-4675410..+8449747` days), so the compiler runs a day-shift interval analysis over the IR between a column and each calendar node:
- bounded and inside the slack: no guard anywhere - nearly every query;
- bounded and outside: decline the calendar entry at compile time, free and not behind any flag - the only half reachable on today's master, and it lands independently of PR apache#62;
- unbounded (a column offset, PR apache#62): the old guard's bytecode - the block apache#73 deleted, cited by commit and line - re-emitted once at the producer's output, only where a calendar node consumes it, behind a `guardDayProducers` emit option whose default a committed number sets.
`NextDay` (+1..7) and `AddMonths` (task 40's month bound) enter the analysis as bounded shifts rather than special cases; pass-through nodes take the hull; `date_add(d, col)` with no calendar consumer is never guarded, since Spark's own `DateAdd` does not check either. Validation restores the two differentials task 51 removed, reshaped around the producer and the compile-time decline, and pins the bound's edges at +-1. Two commits, possibly two PRs: the compile-time half depends only on apache#73, the runtime half also on apache#62.
Also corrects `PLAN_TASK_51.md` section 4 and the milestone's row 52 and section 2.22, which repeated the first version's premise.
### Why are the changes needed?
Task 51 opened a deliberate, recorded correctness gap on the promise that task 52 would close it. The first plan for 52 would not have closed all of it; this one names the whole gap, closes the part reachable today at zero runtime cost, and confines the runtime cost to the one shape that needs it.
### Does this PR introduce _any_ user-facing change?
No. Documentation only.
### How was this patch tested?
Not applicable - plan files only. ASCII checked; prose width matches the sibling plan files.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Fable 5.1)
MaxGekk added a commit to MaxGekk/spark that referenced this pull request Sep 4, 2026
### What changes were proposed in this pull request?
Adds the calendar range guard, moved from every extraction to where a day can actually leave the range. Task 51 (apache#73) removed the per-lane check task 26 ran at every calendar node, on the argument that the range is decidable once; this PR is that decision, in two halves and three commits.
**The compile-time half.** `VarkaExpressionCompiler` runs an interval analysis over the IR under each calendar node (`dayRange`/`calendarInput`): a date column is the project's column contract `0001-01-01..9999-12-31` (two new constants in `VarkaChrono`, derived from `LocalDate` in source), a literal day offset shifts the interval by exactly its value, `next_day` by 1..7, `add_months(n)` by 28n..31n, `last_day` by 0..30, and `greatest`/`least`/`if`/`coalesce` take the hull. An interval that leaves `NARROW_MIN_DAYS..NARROW_MAX_DAYS` declines the entry at compile time with the interval as its reason, so `year(date_add(d, 20000000))`, the query task 51's removed differential used and which fused wrongly since, is residual and the row engine computes it. A producer the analysis does not know declines too, so a future node fails as a residual entry, never as a wrong year. Two refinements over the second version of the plan: absolute intervals instead of slack constants, and no free pass for `last_day`/`add_months` outputs, whose input passed the check at their own arm but whose output can be up to 30 days (31 per month) later.
**The runtime half.** The one shift the compiler cannot see is a column offset (task 38). When a calendar node reads an `AddDays`/`SubDays` with a `ColumnRef` offset, the emitter re-emits task 26's guard block on that producer's own result (`emitProducerGuard`): two compares, ANDed with the node's validity word in the masked body (a null offset must not condemn the batch) and with the epilogue mask, ORed into the per-body accumulator task 51 left in place. The kernel returns `STATUS_CHRONO_RANGE` and the evaluator recomputes the batch on the row engine under `numFallbackBatchesDeclined`, through the projection and filter routes alike. The accumulator is planned only when a body reaches such a producer and `VarkaEmitOptions.guardDayProducers` is on, so every other shape is byte-identical under both settings; the emitter suite asserts it on method sizes, and neither pinned oracle moves. A `date_add` with no calendar consumer is never guarded: it returns what 32-bit addition returns, as Spark's does.
**What a user observes.** No SQL-visible change in values: no null, no exception, no new error. The far shapes that today return a wrong year now return the right one, computed by the row engine, and show in verbose `EXPLAIN` (compile-time case) or in `numFallbackBatchesDeclined` (runtime case).
**Measured** (`VarkaEmitterParityBenchmark`, regenerated at both widths through `dev/varka_bench_regen.sh` after the merge with apache#116, plus a second run to scratch logs; the A/B is adjacent cases from one run):
| case | 256-bit, guard on / off | 128-bit, guard on / off |
|---|---|---|
| `year(date_add(d, off))`, null-free | 2696.7 / 3154.3 M rows/s, **-14.5%** | 1193.3 / 1224.3, **-2.5%** |
| `year(date_add(d, off))`, mixed nulls | 1908.5 / 2183.7, **-12.6%** | 648.6 / 755.0, **-14.1%** |
| `date_add(d, off)` alone (byte-identical control) | 9876.1 / 9888.1, -0.1% | 10447.2 / 10603.1, -1.5% |
The second run put the null-free 256-bit pair 5% apart where the committed run has 14.5%, and its control pair 4% apart, so the null-free cost at 256 bits is between 5% and 15% and a 7 ms row cannot resolve it finer; the mixed-null cost is 13-14% at both widths in every run, over the plan's 10% line, and section 11 explains why: the masked body's guard adds a `VectorMask.fromLong` for the validity AND, a materialization the prediction priced as one lane op. The default is **on**: the cost is paid only by a calendar function over a column-offset `date_add`, a shape the corpus does not contain and whose alternative is a wrong year; off stays the reference variant. The null-free calendar rows did not move (`year` 3455.8); the mixed-null calendar rows moved 6-7% against the file this branch started from with the scalar controls within 0.8%, the machine-day variance the task 54 PR already recorded.
**Docs.** `docs/sql-varka.md` and `SKILLS.md` still described the removed guard as live; both rewritten, with a new `SKILLS.md` lesson. `PLAN_TASK_51.md` closes its regression window in the past tense; `PLAN_MILESTONE_4.md` marks rows 52 and 38 done (row 38 had landed as apache#62 without its marker). The exec node gains a fourth test-only hook, `setEmitOptionsForTesting`, in the shape of the three it already has, so the flag-off reference variant is reachable end to end without a config entry.
**Merged with apache#118 (task 56) as well.** The two compose without a code change: task 56's bounded int-cast offset is a `ColumnRef` offset to this PR's analysis, so `year(d + CAST(i AS INTERVAL DAY))` is column-shifted and guarded at the producer like `year(date_add(d, i))`. The conflicts were the docs paragraph, the two constants' javadoc in `VarkaChrono`, and the two suites' import lines and appended tests; the compiler, chrono, range-kernel, differential, evaluator and exec suites pass on the merge.
**Merged with apache#116 (task 35).** `trunc(date, ...)` landed while this PR was open, as a calendar node with a date-typed output. Its arm now takes its child through `calendarInput` like the seven this PR covered, and the range analysis bounds a truncated date at its input shifted by `[-365, 0]`; the compiler suite holds `year(trunc(date_sub(d, k), 'YEAR'))` at 365 short of `date_sub`'s own last fusing shift and one past it. The parity file was regenerated once more after the merge, since the task 35 pair and this task's pair now share it.
### Why are the changes needed?
Since apache#73 the ghost-fallback contract has had a hole: a day pushed past the narrowed range by `date_add` under a calendar function fused and returned a wrong year silently. `PLAN_TASK_51.md` section 4 recorded the window as accepted and temporary; this closes it, at zero runtime cost for every shape the corpus contains and one guarded producer for the one it cannot bound.
### Does this PR introduce _any_ user-facing change?
No change in any SQL result that was correct before. Results that were wrong past the range are now correct. A calendar function over a literal day shift past the range is residual, visible in verbose `EXPLAIN` with the reason `day range [lo, hi] leaves the calendar lowering's range`; a column-offset `date_add` under a calendar function can decline a batch to the row engine, counted in `numFallbackBatchesDeclined`.
### How was this patch tested?
The standing gate (`dev/varka_gate.sh`): `catalyst/testOnly *Varka*` and `sql/testOnly *Varka*` at both vector widths, the opt-in exhaustive sweeps, `catalyst/doc`, `dev/lint-java`, `dev/scalastyle`, the quote check. New tests: the compiler at +-1 of the bound under every calendar arm, the composition and hull cases and the column-offset admission; the emitter suite's guard in a loop lane, an epilogue lane, under a null offset and a null date, under both settings, with CSE off, and the byte-identity of every unguarded shape; the differential suite's restored out-of-range cases on the projection and filter paths, the flag-off case asserted on the metric only, and the residual literal case with its `EXPLAIN` reason. The parity file regenerated once on an idle machine with its 128-bit companion and provenance.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Fable 5.1)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@ScrapCodes@AmplabJenkins@mateiz