Skip to content

Commit 4979e92

Browse files
estebanzimanyiestebanzimanyi
committed
doc: add PR Reviewer Guide + visibility wiring (PR template + README banner)
Adds doc/contributing/reviewer-guide.md with the canonical PR ordering, tier ranking, dependency chains, and standards checklist for MobilitySpark and its JMEOS dependency. Wires visibility: - .github/PULL_REQUEST_TEMPLATE.md links to the guide so contributors are prompted to update it in any commit that opens / closes / restructures a PR. - README.md gains a 'For contributors and reviewers' section pointing to the guide. Same canonical structure as MobilityDB/MobilityDuck so reviewers landing in any of the three platform repos find the same path.
1 parent fc0670e commit 4979e92

3 files changed

Lines changed: 151 additions & 0 deletions

File tree

‎.github/PULL_REQUEST_TEMPLATE.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<!--
2+
👀 Reviewers: this project's tier ranking, dependency chains and the
3+
standards checklist live in:
4+
doc/contributing/reviewer-guide.md
5+
PR authors: update that file in the same commit as any PR queue change.
6+
-->
7+
8+
## Summary
9+
10+
<!-- What this PR does, in 1–3 bullets. Focus on the "why". -->
11+
12+
## Test plan
13+
14+
<!-- - [ ] Bullet checklist of TODOs for verifying this PR -->

‎README.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,14 @@ The MobilityDB project is developed by the Computer & Decision Engineering Depar
1616

1717
More information about MobilityDB, including publications, presentations, etc., can be found in the MobilityDB [website](https://mobilitydb.com).
1818

19+
### For contributors and reviewers
20+
21+
- Reviewing a pull request? See the
22+
[PR Reviewer Guide](doc/contributing/reviewer-guide.md) — tier ranking,
23+
dependency chains and the standards checklist. Reviewers landing in
24+
any of the three platform repos (MobilityDB / MobilityDuck /
25+
MobilitySpark) find the same canonical structure at the same path.
26+
1927

2028
## Table of Contents
2129

‎doc/contributing/reviewer-guide.md‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
<!--
2+
Copyright(c) MobilityDB Contributors
3+
4+
This documentation is licensed under a
5+
Creative Commons Attribution-Share Alike 3.0 License
6+
https://creativecommons.org/licenses/by-sa/3.0/
7+
-->
8+
9+
# MobilitySpark PR Reviewer Guide
10+
11+
Quick reference for anyone reviewing open pull requests in **MobilitySpark** and its JMEOS dependency.
12+
Updated in the same commit as any PR that changes PR state or adds new branches.
13+
**Last updated: 2026-05-11 — PR #9 extended to 86 UDFs (100% `meos_h3.h` parity) and the portable q02.sql polygon-side prefilter via MobilityDB PR #938 (`geo_to_h3index_set`). All three coordinated PRs source-complete (MobilityDB #938, MobilityDB-BerlinMOD #24, this PR #9); awaiting upstream th3index merge + JMEOS regen.**
14+
15+
---
16+
17+
## How to find this guide
18+
19+
- **In the repo:** `doc/contributing/reviewer-guide.md`
20+
- **Rule:** every commit that opens, closes, or restructures a PR must update this file in the same commit. A one-liner status change is enough; a fuller rewrite is needed when the dependency graph changes.
21+
22+
---
23+
24+
## CI legend
25+
26+
| Symbol | Meaning |
27+
|--------|---------|
28+
| ✅ | All checks green |
29+
| ❌ | Real failure — needs investigation before review |
30+
| ⏳ | CI running |
31+
| ❓ | No CI result yet (doc-only, draft, or external PR) |
32+
| ⚠️ | Non-blocking failure (e.g. macOS/Windows `continue-on-error`, Codacy ACTION_REQUIRED — maintainer overrides in UI) |
33+
34+
---
35+
36+
## Dependency chains — land in this order
37+
38+
```
39+
MobilityDB/MobilityDB
40+
PR #807 / #866 / #893 th3index temporal H3-cell index type
41+
└─► JMEOS regen against MEOS 1.4 master (parallel-session work)
42+
└─► MobilityDB/MobilitySpark
43+
PR #9 perf/spark-mt-and-binary (th3index spatial prefilter — Stage-2 of perf)
44+
45+
MobilityDB/JMEOS
46+
PR #9 JashanReel:fix-tests-using-docker (multi-module Maven layout; needs cleanup review)
47+
└─► PR #12 estebanzimanyi:fix/multimodule-with-split-interface (split JNR-FFI → ARM64/macOS fix)
48+
└─► MobilityDB/MobilitySpark
49+
PR #7 fix/license-main-java (multi-platform CI bootstrap — stacks on JMEOS above)
50+
└─► PR #5 feat/jmeos-1.3-berlinmod-poc (JMEOS 1.4 + BerlinMOD + 99.3% parity)
51+
└─► PR #8 doc/reviewer-guide (this guide — independent, mergeable any time)
52+
```
53+
54+
**PR #11** (`estebanzimanyi:fix/split-meos-library-interface`) was superseded by
55+
`fix/multimodule-with-split-interface` (#12) and **closed 2026-05-10** as part of the
56+
MobilitySpark consolidation pass.
57+
58+
**PR #8** (`SachaDelsaux:JMEOS_v1.3`) is subsumed by PR #9 — external author; recommended for
59+
closure but left in place pending author signal.
60+
61+
---
62+
63+
## Tier 1 — Merge immediately (visibility / trivially reviewable)
64+
65+
| Repo | PR | Branch | Description | CI |
66+
|------|----|--------|-------------|----|
67+
| MobilitySpark | #8 | `doc/reviewer-guide` | **This guide** — visibility wiring (PR template + README banner) — single commit | ✅ |
68+
| JMEOS | #8 | `SachaDelsaux:JMEOS_v1.3` | Subsumed by #9 — recommended for closure (external author; comment posted, awaiting signal) | ❓ |
69+
70+
---
71+
72+
## Tier 0 — Performance (cross-platform th3index unification)
73+
74+
Cross-platform unification: all three benchmarked platforms (MobilityDB, MobilityDuck, MobilitySpark) execute identical portable SQL with the th3index spatial prefilter. PostgreSQL/MobilityDB additionally gets GiST + SP-GiST indexes — the native machinery that the columnar prefilter simulates on the other two platforms. **Land MobilityDB #807 / #866 / #893 / #938 first (foundation + polygon-side public API); then MobilityDB-BerlinMOD #24 (data side); then this PR #9 (consumer side).**
75+
76+
| Repo | PR | Branch | Description | CI | Notes |
77+
|------|----|--------|-------------|----|-------|
78+
| MobilityDB | **#938** | `estebanzimanyi:feat/h3-static-geo-coverage` | Static-geometry → H3 cell set public API: `h3_gs_point_to_cell` (promoted to public) + `geo_to_h3index_set` (POINT/LINESTRING/POLYGON/MULTI*/GeometryCollection in one walker) + `ever_eq_anyof_h3indexset_th3index`. PG SQL wrappers. pg_regress fixtures verified locally on PG 17 build (15/19 pass; 2 intentional ERROR; 2 wait on a pre-existing th3index branch SRID-resolver gap). | ❓ | Stacks on MobilityDB #866. Unblocks the polygon-side BerlinMOD prefilter consumed by this PR's q02.sql. |
79+
| MobilityDB-BerlinMOD | **#24** | `estebanzimanyi:feat/portability-export-th3index` | Extend `berlinmod_portability_export()` to write a 4-column `trips.csv` including `trip_h3` (th3index hex-WKB at H3 resolution 7). Single SQL function change. | ❓ | Data-side, depends on `tgeompoint_to_th3index` from MobilityDB #807 / #866. |
80+
| MobilitySpark | **#9** | `estebanzimanyi:perf/spark-mt-and-binary` | Cross-platform th3index unification: 86 UDFs (Th3IndexUDFs at 100% parity to `meos_h3.h`); load-time `trip_h3` materialisation; portable BerlinMOD SQL (q02/q04/q05/q06/q10) adopts the prefilter directly — including the polygon-side Q2 path via MobilityDB #938's `geoToH3IndexSet` + `everIntersectsH3IndexSet_Th3Index`; load_mbdb.sql adds GiST + SP-GiST indexes; load_mduck.sql + setup/generate_data.sh updated. `preprocessForSpark`'s th3index injection rules removed (now redundant with portable SQL). | ❌ | Source-complete; CI builds will fail until MobilityDB #807 / #866 / #893 / #938 land + JMEOS regen exposes the new symbols (parallel-session `feat/regen-against-meos-1.4`) + MobilityDuck th3index port lands. |
81+
82+
---
83+
84+
## Tier 2 — Cross-repo chain (stack in order: JMEOS → MobilitySpark)
85+
86+
| Repo | PR | Branch | Description | CI | Notes |
87+
|------|----|--------|-------------|----|-------|
88+
| JMEOS | #9 | `JashanReel:fix-tests-using-docker` | Multi-module Maven layout (`jmeos-core/`); MEOS 1.3 API; test fixes | ❓ | **Land first** — needs cleanup (IDE files, binary blobs, *.class, squash 78 commits) |
89+
| JMEOS | #12 | `estebanzimanyi:fix/multimodule-with-split-interface` | Split `MeosLibrary` 1685-method interface into 4 `public static` sub-interfaces for JNR-FFI; removes binary blobs + `.class` + debug files; updates `.gitignore` | ❓ | Stacks on #9 |
90+
| MobilitySpark | #7 | `estebanzimanyi:fix/license-main-java` | Multi-platform CI bootstrap: license check, compile, 51 unit tests on Linux; macOS/Windows non-blocking | ✅⚠️ | Stacks on JMEOS #12 |
91+
| MobilitySpark | #5 | `estebanzimanyi:feat/jmeos-1.3-berlinmod-poc` | JMEOS 1.3 + BerlinMOD Q1–Q17 + TemporalParquet edge-to-cloud pipeline; 37/37 tests | ✅ | Stacks on #7 |
92+
93+
---
94+
95+
## Tier 3 — Fork staging (awaiting upstream merge)
96+
97+
These PRs exist on the fork and are awaiting upstream review after the Tier 2 chain merges.
98+
99+
| Repo | PR | Branch | Description | CI | Notes |
100+
|------|----|--------|-------------|----|-------|
101+
| MobilitySpark fork (`estebanzimanyi/MobilitySpark`) | #1 | `feat/udf-parity-phase2` | Expand UDF surface: 141 new UDFs in 7 classes + JMEOS-1.5 sub-interface fix; 203/203 tests | ✅ | Depends on JMEOS #11 being merged upstream |
102+
103+
### UDF breakdown for fork PR #1
104+
105+
| Class | UDFs | Summary |
106+
|-------|------|---------|
107+
| `ConstructorUDFs` | 18 | Text-literal constructors for temporal/span/set/box types |
108+
| `AccessorUDFs` | 25 | start/end/min/max value, shift, scale, atSpan, atSpanset |
109+
| `SpanAlgebraUDFs` | 23 | Span+set topology predicates + algebraic operations |
110+
| `AnalyticsUDFs` | 12 | tfloat math, tnumber integral/twavg, tpoint length/speed/azimuth/direction |
111+
| `PredicateUDFs` | 36 | Temporal order comparisons + ever/always lifting |
112+
| `OutputUDFs` | 15 | Text serialisation, metadata, instant/sequence navigation |
113+
| `TemporalLiftingUDFs` | 30 | Lifted comparisons (tbool), arithmetic, deltaValue, tprecision, tsample |
114+
115+
Total: 40 pre-existing + 141 new = **181 UDFs**.
116+
117+
---
118+
119+
## Review checklist
120+
121+
For every MobilitySpark / JMEOS PR, verify:
122+
123+
- [ ] PostgreSQL License header on every `.java` file
124+
- [ ] `meos_initialize()` in `@BeforeAll`; **no** `meos_finalize()` in `@AfterAll`
125+
- [ ] Surefire `forkCount=1 reuseForks=false` preserved in `pom.xml`
126+
- [ ] New UDF: `null` input → `null` output (STRICT semantics)
127+
- [ ] New UDF: `DBL_MAX` MEOS return → `null` Java return (NAD sentinel)
128+
- [ ] No large binary data files (> 10 MB) in the commit
129+
- [ ] CI green on Linux before requesting merge

0 commit comments

Comments
 (0)