From 5da48e63f2a040c95682e0e185e0b9586edc338e Mon Sep 17 00:00:00 2001 From: espg Date: Tue, 7 Jul 2026 17:52:08 -0700 Subject: [PATCH 1/3] phase 1 of issue #190 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017w2BCjStojyfaTkw77BfT2 --- src/zagg/index/inline.py | 125 +++++++++++++++++++++-- tests/data/index/atl03_mini.h5 | Bin 50879 -> 88090 bytes tests/data/index/make_fixture.py | 41 ++++++++ tests/test_index.py | 168 +++++++++++++++++++++++++++++-- 4 files changed, 319 insertions(+), 15 deletions(-) diff --git a/src/zagg/index/inline.py b/src/zagg/index/inline.py index 87abaa4..fc3983d 100644 --- a/src/zagg/index/inline.py +++ b/src/zagg/index/inline.py @@ -25,8 +25,13 @@ (``/.parquet``, the PR #159 offsets schema plus the per-dataset decode metadata) after its last group is read. That is how the sidecar store gets populated before a ``sidecar`` backend can serve it (the -issue's deployment progression); coverage is lazy — the datasets this run's -planned reads actually touched. +issue's deployment progression). Coverage defaults to **full** (issue #190): +the manifest carries chunk maps for *every* decodable dataset in the granule, +enumerated from the same front-of-file metadata the read already cached (no +extra chunk GETs), so one build serves any downstream config — the #163 store +design intent. ``write_back_coverage: lazy`` restores the old behavior +(persist only the datasets this run's planned reads touched) for a private +run that wants the smaller manifest. Known h5coro quirk this backend must sidestep: a hyperslice starting exactly on an interior chunk boundary (``k * chunk_len``, ``k > 0``) trips h5coro's @@ -190,12 +195,26 @@ def build_chunk_map(h5obj, path: str) -> ChunkMap: never raises on its own — it just leaves default metadata) and ``ValueError`` for layouts without file-offset storage (compact). """ - from h5coro.h5dataset import INVALID_VALUE, H5Dataset - from h5coro.h5metadata import H5Metadata + from h5coro.h5dataset import H5Dataset ds = H5Dataset(h5obj, path, earlyExit=True, metaOnly=True, enableAttributes=False) if ds.meta.typeSize == 0: raise KeyError(path) + return _chunk_map_from_dataset(h5obj, ds, path) + + +def _chunk_map_from_dataset(h5obj, ds, path: str) -> ChunkMap: + """Assemble a :class:`ChunkMap` from an already-parsed ``H5Dataset``. + + The metadata-only body of :func:`build_chunk_map`, factored out so the + full-coverage walk (:func:`_iter_datasets`, issue #190) can map each + dataset from the single ``H5Dataset`` it already parsed to classify it — + avoiding a second object-header parse per dataset. ``ds`` must be a + non-group (``typeSize != 0``) metadata-only dataset. + """ + from h5coro.h5dataset import INVALID_VALUE, H5Dataset + from h5coro.h5metadata import H5Metadata + dims = tuple(int(x) for x in ds.meta.dimensions or ()) try: dtype = np.dtype( @@ -265,6 +284,65 @@ def _empty() -> ChunkMap: raise ValueError(f"{path}: unsupported storage layout {ds.meta.layout!r} for chunk indexing") +def _iter_datasets(h5obj): + """Yield ``(path, H5Dataset)`` for every dataset in the granule. + + Descends the group tree with h5coro (``metaOnly``, attributes off), so the + whole enumeration is served from the front-of-file metadata block h5coro + already cached — one ranged GET, no chunk data (issue #190; the full walk + stays inside the single ~4 MiB cache line the config's read already paid + for, measured on real ATL03). A node whose ``typeSize == 0`` is a group + (or a typeless link) and is descended; anything else is a dataset yielded + with the ``H5Dataset`` its classification already parsed, so the caller + can build its chunk map without re-parsing the object header. Paths are + absolute (leading ``/``) and visited in sorted order for determinism. + """ + from h5coro.h5dataset import H5Dataset + + def descend(path: str): + H5Dataset(h5obj, path, earlyExit=False, metaOnly=True, enableAttributes=False) + prefix = "" if path == "/" else path.lstrip("/") + "/" + kids = set() + for pp in list(h5obj.pathAddresses.keys()): + if pp.startswith(prefix): + rel = pp[len(prefix) :] + if rel and "/" not in rel: # a direct child of ``path`` + kids.add(prefix + rel) + for kid in sorted(kids): + kp = "/" + kid + ds = H5Dataset(h5obj, kp, earlyExit=False, metaOnly=True, enableAttributes=False) + if ds.meta.typeSize == 0: + yield from descend(kp) + else: + yield kp, ds + + yield from descend("/") + + +def full_granule_maps(h5obj, existing: dict[str, ChunkMap]) -> dict[str, ChunkMap]: + """Chunk maps for **every** decodable dataset in the granule (issue #190). + + The write-back full-coverage payload: enumerate the whole group tree + (:func:`_iter_datasets`) and map each dataset, **reusing** any map already + in ``existing`` (the config's read set, built during the read) so those + stay byte-identical to the read path and aren't re-walked. Undecodable + dtypes (strings/compounds) are recorded with a blank ``dtype`` and skipped + at read by the sidecar consumer (``datasets_from_manifest``) — the + include-and-skip choice, matching that consumer. COMPACT-layout datasets + (data in the object header, no file offset) raise :class:`ValueError` and + are dropped: a ranged read could never serve them. + """ + maps = dict(existing) + for path, ds in _iter_datasets(h5obj): + if path in maps: + continue + try: + maps[path] = _chunk_map_from_dataset(h5obj, ds, path) + except ValueError: + continue # COMPACT (or other offset-less layout): not chunk-indexable + return maps + + def granule_manifest(maps: dict[str, ChunkMap]) -> pd.DataFrame: """Assemble one granule's write-back manifest from its chunk maps. @@ -341,11 +419,27 @@ class InlineIndex(VirtualIndex): """ name = "inline" - config_keys = frozenset({"write_back", "store"}) + config_keys = frozenset({"write_back", "store", "write_back_coverage"}) + + #: Accepted ``write_back_coverage`` values (issue #190). + _COVERAGE = frozenset({"full", "lazy"}) - def __init__(self, write_back: bool = False, store: str | None = None): + def __init__( + self, + write_back: bool = False, + store: str | None = None, + write_back_coverage: str = "full", + ): self.write_back = bool(write_back) self.store = store + # Write-back coverage (issue #190): ``full`` (default, the #163 store + # design intent) persists chunk maps for EVERY decodable dataset in the + # granule — extracted from the metadata the read already cached, so any + # downstream config can be served from the manifest; ``lazy`` persists + # only the config's read set (the pre-#190 behavior), an escape hatch + # for a private run that wants the smaller manifest. Neither changes + # what the READ path fetches. + self.write_back_coverage = write_back_coverage # Chunk maps accumulated across each in-flight granule's groups, # keyed full resource URL -> {dataset path -> ChunkMap} (issue #180: # the worker may hold ``data_source.granule_workers`` granules in @@ -373,6 +467,17 @@ def validate_index_config(cls, index_cfg: dict, data_source: dict | None = None) "index.store is only meaningful for backend 'inline' with " "write_back: true (inline never reads the store)" ) + coverage = index_cfg.get("write_back_coverage", "full") + if coverage not in cls._COVERAGE: + raise ValueError( + f"index.write_back_coverage must be one of {sorted(cls._COVERAGE)} " + f"(got {coverage!r})" + ) + if "write_back_coverage" in index_cfg and not write_back: + raise ValueError( + "index.write_back_coverage is only meaningful for backend 'inline' " + "with write_back: true" + ) # Both read routes accept this backend (issue #170 phase 2): sources # with read_plan.spatial_index take the planned route, read-plan-less # (flat) sources the full-read route -- same compiled addressing seam. @@ -392,6 +497,7 @@ def from_index_config(cls, index_cfg: dict) -> "InlineIndex": return cls( write_back=index_cfg.get("write_back", False), store=index_cfg.get("store"), + write_back_coverage=index_cfg.get("write_back_coverage", "full"), ) def _pending_for(self, h5obj) -> dict[str, ChunkMap]: @@ -460,6 +566,13 @@ def finish_granule(self, h5obj, granule_url: str) -> None: maps = self._pending.pop(granule_url, {}) if not self.write_back: return + if self.write_back_coverage == "full": + # Widen the persisted set to every decodable dataset in the granule + # (issue #190): the read already cached the metadata, so this is a + # metadata-only walk (no extra chunk GETs) that lets the manifest + # serve any downstream config, not just this run's read set. The + # read path is untouched — only what write-back PERSISTS grows. + maps = full_granule_maps(h5obj, maps) maps = {path: cm for path, cm in maps.items() if cm.raw} if not maps: return diff --git a/tests/data/index/atl03_mini.h5 b/tests/data/index/atl03_mini.h5 index 0d4039398202df6cb93d2c2f771bbe2042a72c1e..61ffd2dfacc608caf0a76108841c5997e420e991 100644 GIT binary patch delta 31652 zcmeIbWl$Z>x9E)o5AN>n!QI`1ySuxyae~W6g9i%^!GgOx1b252ZZCQA{LeY>t-AMq zxgY7OHLKTZ8S6bg)q8%m+M~h0Rzbv3f#Ya^R?|jh*!6>l}?TjsLYz&<|^-T<2 z3_(CZLH;JD_-e`;BJe&iYH$#&e|?XTe@p*U=Rxsr4oCSP4cvoZ#yK_lTlqhAPAdM| z@YV6vV?ZKstQtWmK`>B3U;vB&wuz~Yi=n=YrLF1zzY!L25(2a497YZ7BLoQ3zk46E zimxW9k^G1S3Vi*BRg(kHE-CkCFJNy1{w5#Z1maB~-vsJSpx*@MO<>;y?oB?v3H+NN zyb0o)AiW9lo1nZ2>YJd|Osm3xzP0ivLtL2@0!BIF)hMcm!lP8Z75+&Ue-#K{1<>o3v`fqVT4KSeF16XBR1_}YxHS>hk__rB-`P=aWXU%eevE5cR&=vz=Kq8-F zuvKf|3Y_tuQv%fmZV+I?BDnl*APxx(sKubM_WEl+{W-Tk>CgH8k5ujd*GPi^PS#?; zm52T{Yau{@hWY5Q;!%I?TL>U=9R|%q{9p6#KfV3ATK^+mr2I9WAb^f_7;p!ff6d3Y z-V|NYVRrKV+HycpB22LLVxUq!Mhz3u2qfTDed?9~%3&MY!qS}^VQ@Ru?UyomO6ZjbONz5qoM=)h7`axVa$ zHC|7n{GfU3cbKtTk=Z9sf5p}LS!29@Dbd zYw(2#CNV6hj=TO6UHUucefYN3eBM!(-B9$6)62aHY!~A2($TZM`Xo0+0I~M;SKp0p zphAhEIVtP>={EmRIk>yiF~nF(K?C3k%Bme<+_KJib%w|NsdXlsejP^&C_&$hsaRgWx)yypZ{^PEGKRg_w$tEtXh@kKw*w=3~}p^FUf#lt!h16 z^nmKobYV*GC?mnSM`^sUFz5%#f}#QW=J;PRC0ZL=ER}tAU7{ot=;>=d-p<+QaC`D@ zTk~gV0fq>pWgL&6W%%7i@hkAXgsEoNV)JS3y>8GfPnIk$abtn2_6@lRD!SSWqca*J zEad8heXY~nFy3-K$i)j_1=9wE>4JSC2I|1O|k(Fp=R zbh2@6f^0YJnr~NA-{nx7+?=0-%P*XSV0X?9ew6nfDnRKe{$a~R)JG9%;L2rymeuvE=E(k0`PVB}E_X&?Qfdh*?wL{8gkOlzNIJlH_!cz5= z!bL33@FN?Oc_8)^922VPPbsNDxn$k~X~$ncLe^d{dfm-qk2PHpHrfUI_OfjxVY4V# z9TcawSG-u94IG;dbJ1rLe#U{WGC7Ok{h!x^41?M*3bvT0$J?XSiq~$i%Z6ou*eEZD zyl}M0-!BG@tFH1R!>a(y_LaRR9t(?N2D1W6iF?40Se;oahzvR~itUS$pGw;5eZDLX zRr~@AguFQvqP*|~19!(!>jBujm9PXIb9kto;M~}|Zop-?gvHUSus9DZAnx$@+RD(L zIE0J*L%r!l+!%5ek0kn=ywU_D*AcOVg9$WyRakXnZz+mCv~d7^NP~g*AVnYRxGs!+ zc&$5uoK7pnAawQjD`Ns3v^bx_uOjivT6zXaW?kU4vkP$LbssI{oJ5XH;A)c-a$Wmt z8LD22oB#&B5?;sPnU{G`s7j^om6l`>*p9@*$+U;c_rFpF2%>atkeH+xAKExvXV3^? zTQVOiZ&=S3v=0EyY0c~7t47ab7pr^r&DQrHyJ{L6-QOwAHK8T3sz51| zH5RL_r2}tu30bQx&U$ev;5+mOexyjc{pkQKVnG0OFgp^!hD9hc;RA_SSOVP+1PL8d zo~UlB3l|b*SHbW)Sg0mPOIt%#H^x^b1cu|0at;VwlA6u4n-BVEHtHD08lAJAh>ZgC zV=+&{(CioOx&RKrN82jH1^8V9kyeI-Af;j=bPzrj2!9Y~UYx@+5>QEz5&s_GR$0T;IxhsIH7#u5Qof!Sw+=%QabB+u5n%i znv&r8$W9D7Fqh2v*xy2H1+|E?ZSH?Jj@Mh%XRJ-#=sYIJ&}VAbjvu&GfxoW3KtX*i zJ@(Ms8Mzlg{fGy3oD1c9$VNsn7AvhG{u?jmQCFWj?_J3 zc7UFaoa|P#d-~AxA>?)4^YzBlx9#oWWPh#YNmc~~o<_|zBssLvFL2}k094ApA*!GqMIrteUZF5-es2r6A71c>UUELbf z{l4!OgY&x#3V< zDSExKnYG2vxlK=CRx3+*Y0W8yc1u5Ly$De_v)MKYTe<|9uZGAlt)p1unX1E9B}x9> z>LmU5($WRae~{8~qtH3;mz_?XQviGV z)qE_U_O}UB?xX70;AV}sX_Fg%>d;cjif)s>XLDBbT%E8BiH24xLE`fp<6cHvjmWsD2V)(nYWO7S~?hUQQcla)KMjZWkVwg?T)c=EctYhvfwEpQ=LAYn6F@f@nf)S>^{O91d)Z?)hk zTVv4g%a$gTb|KaHXu&GI>)fmSP`R4Mbf{K>GH4P_=1D)leyt@$-Xc}osa=JA^-}8E zfhTw@)ove?7l+V01V>ePt{dxVo#9sFf;_Hl)f0MEYf1Q1oHdg4VZwXxG{~=6I8Bg7 zWIqPC(@C)A;xoTHuW1L3OBdj$cBAx%(Fig=jjG-wLm{kHlf^jt`Lg@;((4W#LAsE& zh%Kyq7mby4tHZJZ>8#UASUXl!-9mWwg9t%~ zplP#VxSx>RaJq_8p!CTeHG^K4*(1z9K~{3D*BvqxPD4~I_Z6$d6-)qlb(+|6w8-N` z4{8h+Ds?*3Jy7@oHR{!gl&2leV`VbAskFOAj}lwzIba}8nCvi}(tAI`P&50lwaIEU zggMTl_SwVp2Af5+t}&HdzM%7DcttDHM877C`r~-cxp$W67X&iK2ex<@{MK2{rR*%z z&Q0YB-R3u52Bjf8lNSOk=3mW|ez_fJUZ68fpFnP@m< zr%RBM>@Yu5xtBwHg8ss$5O1OP4P{YTu8gSs@IE9kwVp3YY7oB+Zt0kCsAJA#=d4n{avxB{}W(q8+{(Z@kZQ8G%zJ zYkpjvr1smFoTbRG(OQBF%cO-MJoQ4OV~oZ5=BN7sU*tzCyV|fHTO0>E=t+v~JdUyy zG{A7CC$3+%E)i+i`K8ELa1n07^FCjeYCTHq%^~hLiVCU&h-b%Eb1y7peJSm{EW;9_ z6*ePc(ptSiK9Qe!4j;uUSU>*O>H1*>sph_sdyz)&ihov!5uRAqA}D67%+mm3;$&y) zyav^^$d6wX7j@_=L-nMjBkp)o+Bqn;Nu7MPnNlZvy!=E8bfVF_K|ocLs`ZDPGVqnz z4~o-JQIdNEtZ&cR!tOd7VZRPN!9CB_yqY{GJ*$r~JH-z1h`%U7^i%Pc>|^yxB;kQO zs~j*|y*j^we8=8Qc$-pyo}9lt@%qCuxY#A^qDzIi*!u^#VvNZ8e>#iJX8^6^9wo6F zV-F0w%xq$Ev9~SoMeO2BgN0+hcaq#Twl4@gRo9IJre7Z)j-Gkf#*ZG?WESTs^BztR zVk2xl_$_Zn#B@1aJYNsFxL8!=pYH;4LXBON`pqGwz|YXSCOG83Uq83>QO8xq40k6G ze5#46wl^DX#@ADA{t`ILh4Uqg2S?x>Gd;tTsL*gz5os4K=t#q)#buf&iL=F1Z;<=L z5-YqB05yJ&*&RNrKausfd;+2-!$6m8#*@(}M@H^7`qNLBx}RGuW1}SL$?D<9ZKs@s z64x7eg%hgK<-n({Rs4NM!Qu#0z%DP5iL6Q9n$>`K+Q7symV!E&;1I=*fd9Bt6-4k2 zF1PRlA2|Nz|D=sX)P4+ZqQ*pY^l?QG#~zs2`JmB$l0vj7iyYWzon9dLRd!?V(6<3Yi4|Ko$*OfW@;l4HI_wO= z(KzTYOU!c`J~g-(mEu5;B)efx0mTS|MyHhC494%Yy}||+Be#cA=0CVzZ4M63mAFqm z048qScl=4qK?IpGH)P7IY34=X#U6LC(d%6J(*O@zyxPndQPk67qsnRG_O-7DBd*;1 zo4h+Wk{7QZ16&@e;2UDiR&Zt#?lbmLf2sF3K$21EVRhrxZ1Gw1llppV5K2Jn|_6|XfCDnV6pwsUwD=ujs=$}G2y2cG2E zZ6%q5VgKI1wNp0Z!(mH+$gr4@`Bk}XB=O>I3F%U9IN|!FyMc(MoLqr*y+UIj-soc; z6Cj$o6$-Mz_;bl8ub9I^ia5f0r$QLW1zk91QD^%t<&JJPqE$fe@%yU2SMflAIe<(o zyYRlPOTU0&J=|ui249Ir`-8%OY!|S9^6)%c+?5< zQ2IDuPoeXm8#q&-B!%tzR5^d0v;$7vYoKX^Q={Ne_*t1IRBbTF0TNfVg)-Hy^!(M6 z!7ryBYV+m#rJq81vY^@{g-2K|8-P#AY);Q(``6szI)zm;OCpPGn9LuE69M5QnzceQ z?7oLd>n?}>5T$lWPMv6qt!D)byw1rn&;@lUlx-XN))I_yn=G-&=1aopY7zK9p-^jY zN_l(v&2T4yft5b=KX#5|r{{@(!YG*FOt9vfysdLAcp0m5(CRQo6?7KsJplBx07iq0 z=&r71lOd;@BR zx{M0d^tVJ%=v|17ZfWo3|IQ$F(*MXX9ceY6Gj*=(-0u?sJ+#z=k&c#e$zD4`1Y09i zu+_XLf0qMY8T>6d+?L()r!&CB?qmWs2y29bsr8%pOE6)?xqL^RtX+*vw=T(*RhT+f zuR(nPHag!`j>*esi{^ucQ}XXcb>s$|~5ChP8)L=x&}&^o2X z&nMO2KJ@69{4(SY;+5QHNG8RWYdY(x#vEcjjBT_Ntu`V*szBta2n7XzDmN9Vz_LnF zBPEi6c$l#ah_Jd>^A7F@^~>Wyt*>A?KVytWgd7s#@dj;uf<1@V&C6Cp#2SknC|Rp= zx`>wa($Nt+%+^@WH`+<@KTMtX{=g?W-p}C@Gg5Xu?F>6ya-hxq7@Wu4t4d$lF8~0q zK9`}g!d+^F>MPtynR5qxRA4-tbc7xL-bH`93@2jj(6ifi3x`X@je)>V>}UyrPX9AL z~PN*c&Owc zt@R!?BwUj%d^f`|A@2VC(!ZsuyY_1Af_Zc@1foQdHfzXi2Z>6 zZ5|l6WJUABf^;A@rNEZ?9z|{)IwFa!i;@;kuPL&?5=@dOe8VPeQjGj*T8Jv6PorP& zCiO@E&l}<7W8oFRYlic9hI9M9U$(+{*q-~?Bll*vveS?jWShN+JkhU@wr7x(C~F)y zvgOXu139kcD+Il!7&C6M&n7!bihZeD1pE}+>aJ0A$N1=CRyaJHq@O_vGFCz}64g0s zuRntREXFht()3>e2dT&d9ic3as&)YVnS-;wwhWTa37Vk|KsaZl>QF!76-kf;QLqOo zD}i$G!RXkc1#+1qbe_)y4!00ZjN4ZgnV z<-c$T+I9--KwJlGyMv+#o%#g9NsqYRi2uE6%FXxpj4^5DisHx*RZP)60G5wVLC>91;!L8X1HBRg{C078oDG1E0K04xK2H@coc^; zO`>f5c_~Iku#v_kQpOX-79^Ghjt~6(hKP!U-~6_@09{a7*aUVTMJq8HrK3<}W9!8% zY*M_2JaUS7Sq?5`)A5b;QFr?^9MMiI{-{t8 zyQvei65b}-8gv~y#)!Gu6E+NjM>eQpx;>tZ z{qX5k+N(rtQY?|LV2PMcCm$@CzqSt$B>ZO06S?;L-Z;l`FCb=Vle~=a`qidG?@I7O zyB^)%`{&}~TVug*4A$s+el*j8&E%r41szg9IS3w$mXCgmHXX#r2JEF=9>)U0TU%rr zNdeEdSJoNPFXn*ld1x02{YAes3g!f>0j0gj406pYSMd^iwB`tSKJeRB^;Az&80)uX z2=4C3Y(2x`-{P`nsm^Rrc_~-;3O7B59j!l%KcHGDcW^mWBGHRmhaD5MeKm9WxvC&l zfj7NXg$3ftAU1p4#I@(mq}d`_zT$)fAOZ;e1bLoxS$5`7quR3xn*6e)MnFJ@tv zu;J2!w8d5i@N(rRx~F${VQ-JPHZnjfb)h2elEw4y!N#3YJ~yh^xG!kJFy#SkOm#o{ zS2|hDs2RiUBd}^tU*ZQBcB1**pks5skb#BFc3`DyENt+ezYtGeP#^TPkVv~4Tf46f zd@$~V41npvu8G9*MU~+(M{LoTRPk~0a??|?bQQ5yUpwzPp6X-3rIZC>pbi6kZv0+u zKa%SC2qDqSY<4DHilf!!6-L>{Sbp}y$qBB5d^?WjaM7zTxqj?)wXHMWaWMFJ*?qgj ztIy`?aq|Jen1ap`N9sjE`WAEO<#T;|;u$0Qqs-3B=WBb`m%V$a?elB5KhJ1QPBbgL z{)=j53K`BDQ#xXqo;X_sWm!OS9Gf)`tdjWXyeDkDFC{AtUUL~+a;OPMa9nPjZt`&m z!k#Nj3g7AY*`fEvEARN@kQFwM7oYS{xgCDi>tF|cL81K-GFlj5g_o&Ss+$;LuFmN? za){3II8%ei2wmwHvvXf)5js-Sz#3vIgh^eLy%9)-C3tGQg!E{~7z==kp$Kf9Ud#Ww z40MbJ;tuMAL4G64jg%L0uC)!e0oR9)SRg2YCWwX>{`&e|M#OCpi*{`W3P|%k_)`U< z0XtR)1E>E{Q-3l*1eDzojI$DBU0)UicM<&Q8_K#Zw>%6wTF2x9h(d)()(^fQbbTAa zgpu~9vP}e1jj0eg^g}>xb7gE^3L%LCya}gt+3bgXrt2c*E+L8{Stg~(RTF*l6~sQbR#(JlAPdJ41DT^P!y8LO2@CYe2J1x#4U<31;LPV?%(Q= z$ajYcN|fD|NIIn*Dt-sxkbmdiqK7b}R@(H1nL?I;fqO|ailPC0SRux5FcexxOq-++ zH40U!iJdr^s>a>@&=zN@`!XIL`jI|>Cf&56suJ65Vob5T_0aos%^fZtVIBkFjPXwU z-r6>#@D#m?+-O%!d#+7+e(e`iWqZ;4%f$kTr`_d4qGQ{2%le8Qf9RM_A1d5G0Q0yfU-B4Ton4 z%VlQ1C~HtVY`$ig^h}`S!@dx4#~Zp8!ae2gw*9h+fxqhppnvh@<8V6{KK?|Ly-N{4 z?!Il*^;HO&@1fOz2s`UXhIRkK?T3rfE_n7V8%c?tLB_V9ovWxN)yjU_7OWdIuHE2i zTrK1>dwDYHp7>9 zzcnejMA}pB4s9$wCais`%(jD9(W7UL7ruiq6`&K2Dd>>*Z5>oqs&m;iW1Q1`-;y;xGy3LK{QU-kVN z>RMszmAIPaX+FA7ZLehBa7S%WE7~!qozF+EkF*N1lC3SknD!3;I)Klc3rb@dpCNg~ zS9`+#EJj-^$quLS^+V_9I)q5<5yOx60hdtIFtNVtXkS8&D|m8kJkq>=F?oS=4r*=t zZ$$_gmskME15Ika6iA`bs2v(oVH=l-9?+vi$h-Yw!ccs-Quz6I0*Q;l!bd-IAx99iht-votkp2r2$C!at@6euwg)zyS_}1{Kni_XZbBWn_pHT)A z;5}zX<|yVM#Sq31U*9JvK7!{P-`Bf`jb~m7q@qvgFtfwg`?8v^=e*G%MNDYtb3-lw z%V+DxT|YcXFOihnoCjt#<7FRoKeWJXN#L+D^+E-|RnxH(QZS}MQnxBYj1n%pp35R) zM>P5PE)}Y+5;kjw`}}otbW5xeZ2+rKJJ!geeS1p;$)JG{92?b5rJ@bltwhn)!BKT;uG0domJ^#w`=BU z^pqfjmJcO(r%o_Yi(f7hru~TQ@Wj~7(=O7(3!@kST> z+8KdirE(lLI&b@+Lv&iej0xwLd^VFQj_MsHW1_p@a?y8oI5!?VkHiot%M3i%K?l!I9GCe+R&)h%eDHWxZ%8?U(gATGtcLC<7u90ocG3b0X2F z8d`y0DuG7sAkkIv1+RQ+$a~*Mye{e(dQYgwGeeSP30uR~>tdFPNGBh)eZ}IFV=WOv zhvRM{WX*vyQVdoZ4SP7#Ew%U1YD{NO*)VHM`(ubKz?@~=r@KY9m=}*0l9v$Jr5#z{ zh-OH{RU_I%a#^I77{DkzI|k{}DPv4+LLnF4J=&E{_`OXD#o#pz=9rFhQwoBBMIAlm zI(D#0UJ=oYpe8=_&Bz6P8y@W>uvNYwn~r9c81NR`$sF9+>7Bkj)?p}K-e|NAp2V0BEvkNu9q9&D ztruSms5*GmBR~Nya*`nHzQ*ogEsWa~90U~)(YG=*c<0Hs$N0L-yuMFC!le47TM2I; zZk#z?MG7RV{`$pHUvJp_2qn>1WVV~EHpk(1gQzf8>2MnOMRK6n0$X#wkpP+(1vw=_ z$-kt;`rr^ZO-o~mhg-k>9wJ1XD9$mQ+A?jS@m_5IN5moEp%3F@ee>Cat&^FZqp7d4 z_v`uHF)Y;CLv&O`azI*Ur20CtsI0WOo|LMtvijOYPuJquB=+gX7A6)-z?T>qS^3cs zKftq^mXhw=MBu^xDJ~X4_LJ9heSLMSyMvEgc#s4L1#w=!p@G@f<_>1?7b(5=;_udn z9UtXJ%1`hB8S4bAbM_lw_UD7Qe?=YNdWNJ5=q~B&KI-fCXDgfn+Fn5fy}*V3OabOZ zzu-ao9k-&69mUDU4hhns2Akqqh11m4%!0?p29n8G7RIv7O?HoG7Q8Db7B;HG2>@r= z?ycT_jO~nmgpqPRVn~|2G7Nxdn}(bGg{CV_5GeqiGP`|}OW5w6^KmIEVADc;TR$nm z_Au{UHH=ihi|QfDdk2NSq`PnU2FhMAew~#e(F1syvypJphN<^K4pyR?N~`+yAn>}P z|IDF;Ofq*sJw&gy5tGL*B^NEc>|WT_2`dvjlDxz`Nw@~0yciT9 z#*MmGC_%J4fHVq=X4OD_@X_ap;3ZWg$6!XfShpzu(|H-QprrwPPf7CUMtSLiEltC_ zJVbb$Q^*%Z`dndIX|NE+hmLhi@&aQ-R+D?f#SlZb@+%N~PBr4Iq5LcYW;&$C+{7U3 zh_(}EiYG^HdP0-Qkxf2ojOHRuQJWb+-!2bryu&$`nAK7*aurdMOPRXRhW`e_3xW(E z%lB`Nhg&e(N#j3RD+;{%dI+m4QlqcR0w!%$sH|kj5!YJ67p#R@cZ}LG93%sm09&wH2~u1$ufo?Qm^e)&DSn13;BMIK-VHa<_qck1@pWzz ztj6MuNb|*kZ7v3s);u*qgN3e3GihEsh0?oZSDqgGo6aH;e6%!O4)cz0E|-HqY3Z;m zZ{p#qTIX&noIStQb3)j(6bfMz+)POUW5>mn)j$L1w55L1@3-u*HtmI_#(Kq$Ta4cX znh(#{y6OwsnUM+>`?eu@Q}P7>SadJ|;(jM~CG6|ZX{hEE{GDQvVGadcGgskE_u}fw z*@M34disW68tJAlEHv{^NC$RYk7iccN1oNMSv)cey|rYY9JkoJJFM0BRWcS}vy4JVDR8 zu~^pc-QvKqjAtO$`GyyoZeV4GXsiz>=}&Z1Uvei0)9wpq>P9o4yMd9kI!Mga5b*e< zEVMkBi3*CMPXVm5LQ<+%)md#9EG#m(-V&JVi1zLN>s7Z(%rM!?Q#DR-6>B9&lcTn} zscpHF;Rp8xNkfNtQGj1CeqH3VB~{5?Z3+z>4l2?a4cQ2q1F#OyRjV7jK(+uT9Q z+u~cfM)J*Q6IFRCz7eAL`_N(2QG>kf+WJ7x@8v02H)Hd&^}8n&C{a;id3dv*ep|tp zu+WiGlAYV=s;srHm20x}Z;$&7TcTe+$u?Z z-*DuKUJKfnghS}Z=X-Li0khd?oR1L#Nu+ue^JR~Vhoo+#`|fxW$ri=Jd1L;)wMSkZ zesp9aKSg88R?OLVT6S+~Au}jcg2^mzQe^?06?acjM!rOf{vNrLuBIb?3*M9&0alZFX~kF`>pja z9?mTLOfgsS7HPs;1)#&0bBv%+)p4?h-|+5@vhNeG%-dVB9GVmzfeybuH6?Tdg!urm z2utqlJLz($bXG7_TZfG<91lp;tm)!8Dh_wir8|Lkzey4TP#R4XJ5MT4uq46w?X8|A(n2BNBV1I&7bk{}g%V zPg5mC`1EN~F!INaVi_98>A(;0nbYKft<*yckMfzHVbXWW{BkmW`xQM|tW~3CRe>fh zKJbu$cHRe3>#VD!!*KOpGdx*bp_WS%ptxc&5r?VWw(uYZpEMz>b#S zNoG4Hcs@_{r0=SbTcKXIPWwroN%>fBLeSnY5RCo|7Uzo!AQB^Jk6|!#utNVxpf0<@ zN@MJ1i;0lg6!$Vyn=n%@L$?{^HCx}yGH|lWGU@!4OQ77Es}-`q7?W}L#PG1uHw@__ z+a>#@ZFK-}g8j!&t? zC#&7cWBk%RysYQXw-~iB?G*_>9qdiEk{Ndh=h-uE2*oKXzQGD1j6MmG!VVs#$Y5n^ zUoX)$_RKoR=o}F{_JY zSK1}#tBZ3~MqMT1nY>>?>BS`GwQoz+%QSaMHG$`OsoW22XO_;ueeRt^IP_TyrG4!@ z?C^`8EA+@A7b3)Ed6Gq<14P;e8(2dO>}7R9^QaOWrvK2ur0*moWs^F6FoHTensyMM zigISG0jD1eQ0Kl9VIC@ChmxjRT{BFMtWZHniX7g&fh3102hLKIypFd>tF4&X_)p?Y5FwEB9l zG?nM3{`i7R&>YCtn3EDEj#wmWr)Fy{dh0yD%|QHwBSiJ|Jx2!nT37>fp2QpBRJFmK z+Anpk&EolKg}no{UDf$mcUT{^Ww;pCzX)Ri&f^omNk8FoiA7RLvVOLr|$ zg#Rpxg5Wx z1wIgFrtDyc%X65H9wpy3plj4OKv#Cch4D;GqoUb{}x$(@msOQC&np|)1 z_dS<~d#>BNvp1)0oy0@)+rN*yE*Dei7spOtCUI$lf$8cX6aX4(@<zhXa59IcNiId9dvL1>zB#v=Y{LnN%$x8 z#kb`kIKSy<(m(USjP^7fNT1_DqCB-_9D;x= z9~e@30WtIv`C7%@2EWV(x9;qxux9Btq5iw=D_w-RH*ARL2#E{28e9oA1pvovs=?yZ`)24~YH;pN>_mQq3wkzM zs*D~B4m9$x|b{6 zOKAEi-QKI&a481T>#jwg(qk%!6I25hVMv~LmZ*gQ<0A-5)q-ptiXO$UT*B2O$W?8% z0f5(M`EGEY_CdN`r7PSNPT{UZh)eNDc=nA91)ethRgQmPhRPs?*CHi0;3GtY9~YmK zFnJ{OEG07FgYB+(`F_fzK(^u-N$@q;4yyZngdX77Wy{S0ZEF+gp^Y^yq_j}qT=gBU)zks_*iF|frP$3!$V$?9zhOQ5B%@B2eEKTu> zSFj?S)!EpH^*$ez=!DGr2;49vMWx}zchylH-X#jPZH{6N+$M6 z_XBCX=FzAWiy?hEtu&m>0u>=piwjlq19FpbVo<6i1xaI~I%D*>AhP^$Gn z`<=>ct4kf(*47cl3{=kGC&)5sm! z_o@4)WYRaK^ef{kp1|}AN-((M6xXt84;}U`8p>9 zuz`X54c>6w$4Q#8ZOLs;kZUh|dZ`5WB%q#ifjDrZRSK>;pg#^R*peZs_ z`H9wD4o;^p3l#PueU(W)NG!c@i~?#x*OMYFDZQ=f&@RS(oY{%Tw29 z4acCBnafz`CGsjN70d-E@F@|2c73(3S{P z)sA7Uc#xKPLY=u)Oe^J%()M2;TDokJ*2za?wX-Wg?T;QV!zY4uA4;eC? zLfaF*<{+VQAIjlTaM6=ewtvrWDXHI3w)R$6c%5jd4o|!9-MKDb2?Ex@`H*~uwc-}X zaAP0s?Raqx7cMOs^fnuHe&trTFI2SVfinMzyNNx; zNX1FFsp>*ULCwa;Q+a+^UYJ`9v9z$l!FuWI<8`_>vozLuali#2AclGS#s?hk?aQ3@ zxG%xKbv?;d;pOmszl0ZuBxMGnE{{&b9n4*2&pt}b-Ac^8Ru(=8tO*<@LS%dBrPH-) zm`9!{tyw#cU$t{M$R_H1uN!=*x=l(gYScfPf9Tz_wyEOX_oO~_yxGG^CYWrrIvTlY z*8k!WcV8V*EaOoR7{URzR_t}17PMCy>?36q*A7`UT&2bF6x$EK)Lkg3YZz8dU#Lu! zZ_kkfv^Su*R`8$+0?TUlqJ)D5eBQ+xAOqlF$_dz!PYmIJ16hm3Q+@Mne zub_u!wdr<9+x1cI+{LF3x0~3iQ83RUwnYM-u}Azyx%~4C`90^9kXK{3h!mZvGtZs2 zL~6fEm0ndq9*ljfS``IhTzQ)z`+d-w)mUwJglhwRkuQu+KhRUB;G%7`?2I`5&RM>= zf&WMLBdt-XVyEUwOwkzJi})E5ZClRV^OP;K@iM{S58D0HT#j=^!G_W z0(}5n-lJUcm-_ZGT}<-LlvcK4?tu+~lM3nCKEfMYbFpt2Z4-=3!<%dup+1X?jB3%8 zcip@ElMKrjYvs2QE}IdzW_1!j!OfS|hgU9_s<&D+juYlW(FGq1mm!ztoV1`V2%Eh3 z;jM7B7n>g_4je$S=`hI^an zXvita$|{o+vn;H2RrT&p*XI|Au#r*W3i87v)KmBp z6kT6HfetW8utb0HP7wNPAd{`$pJEu-`oBJ+>S`!3SiyfQ^wt2+?b-j&059+y23i>S z;V%jNA0D{*Uwi3amiP~)`zK-iF9`5|;B|koS?x@`hfvm6ksy<@zu$H5L+Om3l;pQ!siQt%{F@T)2Ufo+nG4|Ln@Z~kUc9NC3m|en0oe5g zkoiqXUe+k(??M81%Tz%vfJ#dAH7(@|A58wALe+1#_7f8`Cq45g`cG^sOrN+}KXJ2t z(yF;Y=mLAYttdc6^;|IR2jF%M5GL>PpP>Lk0_=pxjBkeq>%9c#)>6Ri{g)pU;U7QD z74YKCPkZ&B`a*yGa#uj1Itm#3|MCa?^~YZU|N4o5oqzo^S2Y`T70_`1V^9Jw ztjNKQp+RdVjUd2K|5x-|S`ic&V1y6%Kgjp^YPLT>H`w(R@Uc4|xDEmNHbgo99vDb7 z?4Kd}XHbGGL;aKOc*~Z;{K>-o2Mp&{_{Ws|x6Lrzo0;@apGm)K9=jny!Pu^AzxX)=>n)_7?r2$H9-_L4m7qM*jmJ$Hn;jQ~MVm_rUvW{_pVdHj=;P|Kj6KRDaF? z;N!{+f9-$qaVXZm=Kl^Kf8qLD{x3d$Bk^Kbh z&3}`o1^-uu9Oj$KU$B$t4baK&mbuk73F}AmJak97huUOPX@zt1bJS-8+-<`Y#sRkN!3W3q7zr`fR z01{cm{6pP>{}b`~&(0E*{{d>5{@w9|+8-16hrB&hear6r=?nkA`uexlpXPsjsBarI z;Bq%Uw(Gyw0PWuz|52&&)tPVTGBhX>uoE8>=1<(|Kj{N-zL$y{6zrdlq5ln<4gOD< f>AzvM{}ZVCPi2&U5B5*x#&kI=5RkYx==c8vbEAuo delta 965 zcmZXSUr19?9LIOgbi4P?Z5gGj@NPv+DR3)8qaargDLs_^acYoAf3$=(%Lj>y^dT+6 zKi3~9M(FI{gWK+QIpP+AoBa_COnOKN%cSSpBFG0J>)hSD8FU_gpL4$ae!ufO=l6RU z&W?3!`--%&8I82N8zT$6z=arTz`S~h*t9bkVKD3VY<<_PNiC#6v!b5!Jc6_WHm!xY z^e&8?(7~3oX{RDVvcht6*>jzKkQ8R=u1yB~8YrFw#jXs)&3^(o7!Y+Ltw_Xg~k?i<*3vqeY z5VWuxpg>}9sRs(Bdbvn$!lTr`;FUeK&#!AD1t*aabI$a;ybvR$|GfYbEO(@BN^wdT zH=>?naYbj;96jnKX}|SL<-c+o{5a7HpAA=XvNRSoBsnTbJj_inc;K46i{~(yzeRVP zP4#dPL7I|(4n9OIc%d4S5gz}l26L20o1i#qMI~^Y+EZ|q+E(bHb`Zp<6-V7LALa2U zHzX;3yNT1Yq)U}ie4CbWA(eFVtOeKK1;;Fp%kIGmirfd_rTEqjy%e*35TbYjFi+7w z07;6oLtuW(<9(07L17x9nZN4D!MK!79PhjG@t?=wKML642Ui6R@=z+iYiH*&Ix?m7q}X3)XLCbo_rr=@5`rqKS7-DNMQ;m%kLp9JZi8t> zpA3;7bt1#9H$OFV6)$1?#2G0*mKwXv{C-1Xt6W_&X&M?VGe@_D>XidYTEgz%&ULNM?b1&uPj diff --git a/tests/data/index/make_fixture.py b/tests/data/index/make_fixture.py index 223542f..d2734fd 100644 --- a/tests/data/index/make_fixture.py +++ b/tests/data/index/make_fixture.py @@ -18,6 +18,19 @@ ph_index_beg,segment_ph_cnt}`` — small, **contiguous** (exercises the pseudo-chunk path of ``build_chunk_map``). +Datasets the shipped config never reads, present so the write-back +full-coverage walk (issue #190) has something beyond the read set to cover: + +- ``/{beam}/heights/delta_time`` — chunked (a non-read chunked dataset), + ``/{beam}/geolocation/segment_id`` — contiguous (a non-read pseudo-chunk). +- ``/ancillary_data/atlas_sdp_gps_epoch`` — a top-level (non-beam) group the + read never descends into. +- ``/ancillary_data/data_start_utc`` — a fixed-length **string** dataset: an + undecodable dtype the full walk records (``dtype == ""``) and the sidecar + consumer skips at read (include-and-skip, issue #190). +- ``/ancillary_data/control`` — a **compact-layout** dataset (data lives in + the object header, no file offset): the walk skips it at write. + Geometry is deterministic: 20 segments of 128 photons each, except segment 8 which is EMPTY (``cnt == 0`` and ``ph_index_beg == 0``, the real ATL03 empty-segment marker — issue #116), for 2432 photons total → 10 chunks of @@ -64,9 +77,26 @@ def beam_arrays(lat_offset: int): "geolocation/reference_photon_lon": 0.001 * ibeg + lat_offset, "geolocation/ph_index_beg": ibeg, "geolocation/segment_ph_cnt": counts, + # Not in the shipped read set (issue #190 full-coverage walk fodder): + # a chunked dataset and a contiguous one the config never touches. + "heights/delta_time": (0.0001 * i).astype(np.float64), + "geolocation/segment_id": np.arange(N_SEG, dtype=np.int32) + 1, } +def _add_compact_dataset(f, name: str, arr: np.ndarray) -> None: + """Create a COMPACT-layout dataset (data in the object header, no file + offset) via h5py's low-level API — the write-back walk skips these.""" + import h5py + + space = h5py.h5s.create_simple(arr.shape) + dcpl = h5py.h5p.create(h5py.h5p.DATASET_CREATE) + dcpl.set_layout(h5py.h5d.COMPACT) + tid = h5py.h5t.py_create(arr.dtype) + dsid = h5py.h5d.create(f.id, name.encode(), tid, space, dcpl) + dsid.write(h5py.h5s.ALL, h5py.h5s.ALL, np.ascontiguousarray(arr)) + + def main(out_path: str = "tests/data/index/atl03_mini.h5") -> None: import h5py @@ -82,6 +112,17 @@ def main(out_path: str = "tests/data/index/atl03_mini.h5") -> None: compression_opts=(4 if chunked else None), shuffle=chunked, ) + # A top-level (non-beam) group the read never descends into, holding + # the edge dtypes/layouts the full-coverage walk must handle (#190): + # a plain contiguous array, an undecodable fixed-length string, and a + # compact-layout dataset. + f.create_dataset( + "ancillary_data/atlas_sdp_gps_epoch", data=np.array([1.198800018e9], dtype=np.float64) + ) + f.create_dataset( + "ancillary_data/data_start_utc", data=np.array(["2018-12-25T02:42:52Z"], dtype="S20") + ) + _add_compact_dataset(f, "ancillary_data/control", np.arange(4, dtype=np.int32)) if __name__ == "__main__": diff --git a/tests/test_index.py b/tests/test_index.py index cad23ee..2b072cc 100644 --- a/tests/test_index.py +++ b/tests/test_index.py @@ -541,6 +541,22 @@ def test_missing_dataset_raises_keyerror(self): with pytest.raises(KeyError, match="nope"): build_chunk_map(h5obj, "/gt1l/heights/nope") + def test_string_dataset_recorded_with_blank_dtype(self): + # Undecodable dtype (issue #190): the chunk map is still built (valid + # file offsets) but dtype is blank -- the sidecar consumer skips it at + # read while the manifest keeps it covered. + h5obj = _open_fixture() + cm = build_chunk_map(h5obj, "/ancillary_data/data_start_utc") + assert cm.dtype == "" + assert len(cm.raw) == 1 # contiguous string -> one pseudo-chunk + + def test_compact_dataset_raises_valueerror(self): + # COMPACT layout stores data in the object header (no file offset), so + # it cannot be chunk-indexed -- the full-coverage walk drops it. + h5obj = _open_fixture() + with pytest.raises(ValueError, match="unsupported storage layout"): + build_chunk_map(h5obj, "/ancillary_data/control") + def test_starts_on_boundary(self): h5obj = _open_fixture() cm = build_chunk_map(h5obj, "/gt1l/heights/h_ph") @@ -1100,6 +1116,40 @@ def test_valid_write_back_block(self, tmp_path): assert isinstance(backend, InlineIndex) assert backend.write_back is True assert backend.store == str(tmp_path) + assert backend.write_back_coverage == "full" # #190 default + + def test_coverage_defaults_full(self): + assert InlineIndex(write_back=True, store="s3://b/p/").write_back_coverage == "full" + + def test_coverage_bad_value_rejected(self): + with pytest.raises(ValueError, match="write_back_coverage must be one of"): + validate_index_config( + { + "backend": "inline", + "write_back": True, + "store": "s3://b/p/", + "write_back_coverage": "partial", + } + ) + + def test_coverage_without_write_back_rejected(self): + with pytest.raises(ValueError, match="write_back_coverage is only meaningful"): + validate_index_config({"backend": "inline", "write_back_coverage": "full"}) + + def test_lazy_coverage_from_config(self, tmp_path): + backend = index_from_config( + PipelineConfig( + data_source=_fixture_data_source( + index={ + "backend": "inline", + "write_back": True, + "store": str(tmp_path), + "write_back_coverage": "lazy", + } + ) + ) + ) + assert backend.write_back_coverage == "lazy" class TestGranuleManifest: @@ -1200,6 +1250,24 @@ def _expected_datasets(self, beams=("gt1l", "gt2l")): f"/{beam}/geolocation/{name}" for beam in beams for name in geoloc } + def _full_datasets(self, beams=("gt1l", "gt2l")): + # Full write-back coverage (issue #190): the read set PLUS every other + # decodable dataset in the fixture — the non-read chunked/contiguous + # arrays, a top-level ancillary group, and an undecodable string + # (recorded, skipped at read). The COMPACT ``control`` dataset has no + # file offset and is NOT in the manifest. + extra = {f"/{beam}/heights/delta_time" for beam in beams} | { + f"/{beam}/geolocation/segment_id" for beam in beams + } + return ( + self._expected_datasets(beams) + | extra + | { + "/ancillary_data/atlas_sdp_gps_epoch", + "/ancillary_data/data_start_utc", + } + ) + def test_round_trip_to_local_store(self, tmp_path): store = tmp_path / "zagg-index" / "ATL03" / "007" # created by open_object_store df_out, meta = self._run( @@ -1210,7 +1278,19 @@ def test_round_trip_to_local_store(self, tmp_path): assert manifest_path.is_file() df = pd.read_parquet(manifest_path, engine="fastparquet") assert list(df.columns) == list(MANIFEST_DTYPES) - assert set(df["dataset"]) == self._expected_datasets() + # Full coverage (issue #190): every decodable dataset, not just the + # config's read set — which is a strict subset now. + covered = set(df["dataset"]) + assert covered == self._full_datasets() + assert self._expected_datasets() < covered # strict superset of the read set + # Spot-check datasets the config never reads are present... + assert {"/gt1l/heights/delta_time", "/gt2l/geolocation/segment_id"} <= covered + assert "/ancillary_data/atlas_sdp_gps_epoch" in covered + # ...the undecodable string is recorded with a blank dtype (skipped at + # read by the sidecar consumer)... + assert set(df[df["dataset"] == "/ancillary_data/data_start_utc"]["dtype"]) == {""} + # ...and the COMPACT dataset (no file offset) is dropped. + assert "/ancillary_data/control" not in covered h5obj = _open_fixture() cm = build_chunk_map(h5obj, "/gt1l/heights/h_ph") got = df[df["dataset"] == "/gt1l/heights/h_ph"].sort_values("chunk_idx") @@ -1250,15 +1330,20 @@ def test_coverage_deterministic_for_empty_groups(self, tmp_path): ) assert meta["error"] is None df = pd.read_parquet(store / "atl03_mini.parquet", engine="fastparquet") - assert set(df["dataset"]) == self._expected_datasets() + # gt2l's read returns None before the read seam, but full-coverage + # write-back enumerates the whole granule at finish_granule, so both + # beams (and the shared ancillary datasets) are covered regardless of + # which shard matched — concurrent shards write identical manifests. + assert set(df["dataset"]) == self._full_datasets() def test_manifest_byte_identical_to_direct_chunk_maps(self, tmp_path): - # Pins the sidecar-build product (issue #185): the persisted manifest - # is byte-identical to one assembled directly from build_chunk_map - # over the deterministic per-group coverage — the selection datasets - # included — so the default path's skip-the-walk heuristic cannot - # erode what write_back runs persist. - from zagg.index.inline import write_manifest + # Pins the sidecar-build product (issues #185/#190): the persisted + # manifest is byte-identical to one assembled directly — the read set + # from build_chunk_map (as the read path builds it) widened to full + # coverage by full_granule_maps — so neither the default path's + # skip-the-walk heuristic nor the full-coverage walk erode what + # write_back runs persist. + from zagg.index.inline import full_granule_maps, write_manifest store = tmp_path / "via-backend" df_out, meta = self._run( @@ -1266,7 +1351,8 @@ def test_manifest_byte_identical_to_direct_chunk_maps(self, tmp_path): ) assert meta["error"] is None h5obj = _open_fixture() - direct = {p: build_chunk_map(h5obj, p) for p in self._expected_datasets()} + read_set = {p: build_chunk_map(h5obj, p) for p in self._expected_datasets()} + direct = full_granule_maps(h5obj, read_set) write_manifest(granule_manifest(direct), str(tmp_path / "direct"), "atl03_mini") assert (store / "atl03_mini.parquet").read_bytes() == ( tmp_path / "direct" / "atl03_mini.parquet" @@ -1298,6 +1384,70 @@ def test_finish_granule_drains_pending_when_off(self): backend.finish_granule(object(), "s3://b/g.h5") assert backend._pending == {} + def test_lazy_coverage_persists_only_read_set(self, tmp_path): + # The escape hatch (issue #190): write_back_coverage: lazy reproduces + # the pre-#190 behavior -- the manifest carries exactly the config's + # read set, none of the non-read datasets. + store = tmp_path / "lazy" + df_out, meta = self._run( + tmp_path, + { + "backend": "inline", + "write_back": True, + "store": str(store), + "write_back_coverage": "lazy", + }, + ) + assert meta["error"] is None + df = pd.read_parquet(store / "atl03_mini.parquet", engine="fastparquet") + assert set(df["dataset"]) == self._expected_datasets() + assert "/gt1l/heights/delta_time" not in set(df["dataset"]) + + +class TestFullCoverageWalk: + """Issue #190: write-back full-coverage enumeration + assembly.""" + + def test_iter_datasets_enumerates_whole_granule(self): + from zagg.index.inline import _iter_datasets + + h5obj = _open_fixture() + paths = {p for p, _ in _iter_datasets(h5obj)} + # Every dataset in the fixture -- both beams, the ancillary group, the + # string and compact datasets (groups are descended, not yielded). + assert "/gt1l/heights/delta_time" in paths + assert "/gt2l/geolocation/segment_id" in paths + assert {"/ancillary_data/atlas_sdp_gps_epoch", "/ancillary_data/control"} <= paths + assert "/ancillary_data/data_start_utc" in paths + assert len(paths) == 23 # 16 read set + 7 extras (control incl.) + + def test_iter_datasets_yields_usable_datasets(self): + # The H5Dataset yielded alongside each path builds a chunk map + # byte-identical to a fresh build_chunk_map (no second parse needed). + from zagg.index.inline import _chunk_map_from_dataset, _iter_datasets + + h5obj = _open_fixture() + ref = _open_fixture() + for path, ds in _iter_datasets(h5obj): + if path == "/ancillary_data/control": + continue # compact: raises in both routes + a = _chunk_map_from_dataset(h5obj, ds, path) + b = build_chunk_map(ref, path) + assert a.raw == b.raw and a.dtype == b.dtype and a.dims == b.dims + + def test_full_granule_maps_reuses_and_widens(self): + from zagg.index.inline import full_granule_maps + + h5obj = _open_fixture() + read = {"/gt1l/heights/h_ph": build_chunk_map(h5obj, "/gt1l/heights/h_ph")} + maps = full_granule_maps(h5obj, read) + # The prebuilt map is reused verbatim... + assert maps["/gt1l/heights/h_ph"] is read["/gt1l/heights/h_ph"] + # ...the walk widens to the non-read datasets... + assert "/gt1l/heights/delta_time" in maps + assert "/ancillary_data/data_start_utc" in maps # string kept (blank dtype) + # ...and the COMPACT dataset is dropped (no file offset). + assert "/ancillary_data/control" not in maps + class TestInlineInterleavedGranules: """Issue #180 phase 1: with granule-level read concurrency the worker may From 924c1fa929bec1d7574e8c12eb27df0d6817b902 Mon Sep 17 00:00:00 2001 From: espg Date: Tue, 7 Jul 2026 17:54:39 -0700 Subject: [PATCH 2/3] phase 2 of issue #190 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017w2BCjStojyfaTkw77BfT2 --- tests/test_index.py | 110 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/tests/test_index.py b/tests/test_index.py index 2b072cc..5eb9176 100644 --- a/tests/test_index.py +++ b/tests/test_index.py @@ -1600,3 +1600,113 @@ def test_same_basename_granules_do_not_collide(self, tmp_path, caplog): writes = [r for r in caplog.records if "inline write-back" in r.message] assert len(writes) == 2 assert (store / "granule.parquet").is_file() + + +# --------------------------------------------------------------------------- +# Real-granule validation (issue #190): the full-coverage walk on an actual +# ATL03 granule -- ~1,000+ datasets vs the ~48 lazy read set, previously +# covered datasets byte-identical, and the build-time delta. Gated behind the +# slow marker AND the presence of a local granule (the ~/ignore files are not +# in the repo), so CI (which has neither) skips it. +# --------------------------------------------------------------------------- + +_REAL_ATL03_DIRS = ( + Path.home() / "ignore" / "atl03_1336_r05", + Path.home() / "ignore" / "zagg_neon_atl03_test_shard" / "granules", +) + + +def _real_atl03_granule(): + for d in _REAL_ATL03_DIRS: + hits = sorted(d.glob("*.h5")) if d.is_dir() else [] + if hits: + return hits[0] + return None + + +_REAL_GRANULE = _real_atl03_granule() + +# The ATL03 tdigest/gain_bias read set: 8 datasets x 6 beams == 48 (issue #190). +_ATL03_BEAMS = ("gt1l", "gt1r", "gt2l", "gt2r", "gt3l", "gt3r") +_ATL03_READ_SET = tuple( + f"/{b}/{d}" + for b in _ATL03_BEAMS + for d in ( + "heights/lat_ph", + "heights/lon_ph", + "heights/h_ph", + "heights/signal_conf_ph", + "geolocation/ph_index_beg", + "geolocation/reference_photon_lat", + "geolocation/reference_photon_lon", + "geolocation/segment_ph_cnt", + ) +) + + +@pytest.mark.slow +@pytest.mark.skipif(_REAL_GRANULE is None, reason="no local ~/ignore ATL03 granule") +class TestFullCoverageRealGranule: + def _open(self): + from h5coro import filedriver + from h5coro import h5coro as h5c + + return h5c.H5Coro(str(_REAL_GRANULE), filedriver.FileDriver, errorChecking=True) + + def test_full_covers_thousand_plus_and_read_set_byte_identical(self): + import time + + from zagg.index.inline import full_granule_maps + + # Lazy read set (the pre-#190 coverage): time it and keep the maps. + h_lazy = self._open() + t0 = time.perf_counter() + read_set = {p: build_chunk_map(h_lazy, p) for p in _ATL03_READ_SET} + t_lazy = time.perf_counter() - t0 + assert len(read_set) == 48 + + # Full coverage, reusing the read set (as finish_granule does). + h_full = self._open() + read_reused = {p: build_chunk_map(h_full, p) for p in _ATL03_READ_SET} + t0 = time.perf_counter() + full = full_granule_maps(h_full, read_reused) + t_full_walk = time.perf_counter() - t0 + + persisted = {p for p, cm in full.items() if cm.raw} + # ~1,000+ datasets vs 48 -- a strict, order-of-magnitude widening. + assert len(persisted) > 1000 + assert set(_ATL03_READ_SET) < persisted + + # The previously-covered datasets are byte-identical (no regression): + # full reuses the read-set maps verbatim, so the manifest rows match a + # standalone build_chunk_map exactly. + ref = self._open() + for p in _ATL03_READ_SET: + a = full[p] + b = build_chunk_map(ref, p) + assert a.byte_offset.tolist() == b.byte_offset.tolist() + assert a.nbytes.tolist() == b.nbytes.tolist() + assert a.dtype == b.dtype + + # Build-time delta: the incremental walk cost over the lazy read set. + # Loose upper bound only (timing is machine-dependent); the measured + # value is reported in the PR body. + print( + f"\n[#190] full={len(persisted)} datasets vs lazy=48; " + f"lazy build {t_lazy * 1000:.1f} ms, full-walk add {t_full_walk * 1000:.1f} ms" + ) + assert t_full_walk < 5.0 + + def test_manifest_roundtrips_and_stays_within_one_cache_line(self): + # The full walk is metadata-only: it must not pull chunk data. h5coro + # caches in fixed-size lines; the whole enumeration + mapping should + # stay within the front-of-file block the read already fetched. + from zagg.index.inline import full_granule_maps + + h5obj = self._open() + full = full_granule_maps(h5obj, {}) + df = granule_manifest(full) + assert list(df.columns) == list(MANIFEST_DTYPES) + assert df["dataset"].nunique() > 1000 + # One 4 MiB line covers all dataset metadata (measured on real ATL03). + assert len(h5obj.cache) <= 2 From d0efbc4877bdcd470c2cb6b5c2650f598f69a813 Mon Sep 17 00:00:00 2001 From: espg Date: Tue, 7 Jul 2026 18:03:32 -0700 Subject: [PATCH 3/3] fold review: per-dataset tolerance + cycle guard (issue #190) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017w2BCjStojyfaTkw77BfT2 --- src/zagg/index/inline.py | 50 ++++++++++++++++++++++++++++------------ tests/test_index.py | 23 ++++++++++++++++++ 2 files changed, 58 insertions(+), 15 deletions(-) diff --git a/src/zagg/index/inline.py b/src/zagg/index/inline.py index fc3983d..a0e8e26 100644 --- a/src/zagg/index/inline.py +++ b/src/zagg/index/inline.py @@ -294,29 +294,43 @@ def _iter_datasets(h5obj): for, measured on real ATL03). A node whose ``typeSize == 0`` is a group (or a typeless link) and is descended; anything else is a dataset yielded with the ``H5Dataset`` its classification already parsed, so the caller - can build its chunk map without re-parsing the object header. Paths are - absolute (leading ``/``) and visited in sorted order for determinism. + can build its chunk map without re-parsing the object header. Each node's + object header is parsed exactly once: parsing a group path populates its + *direct* children into ``h5obj.pathAddresses`` (h5coro traverses one level + to resolve the path), so the classification parse doubles as the group's + enumeration parse. A ``seen`` set of object-header addresses bounds the + recursion against a (non-ICESat-2) hard-link cycle. Paths are absolute + (leading ``/``) and visited in sorted order for determinism. """ from h5coro.h5dataset import H5Dataset - def descend(path: str): - H5Dataset(h5obj, path, earlyExit=False, metaOnly=True, enableAttributes=False) + def child_paths(path: str) -> list[str]: prefix = "" if path == "/" else path.lstrip("/") + "/" kids = set() for pp in list(h5obj.pathAddresses.keys()): if pp.startswith(prefix): rel = pp[len(prefix) :] if rel and "/" not in rel: # a direct child of ``path`` - kids.add(prefix + rel) - for kid in sorted(kids): - kp = "/" + kid + kids.add("/" + prefix + rel) + return sorted(kids) + + seen: set[int] = set() + + def visit(path: str): + for kp in child_paths(path): ds = H5Dataset(h5obj, kp, earlyExit=False, metaOnly=True, enableAttributes=False) - if ds.meta.typeSize == 0: - yield from descend(kp) + if ds.meta.typeSize == 0: # group (or typeless link): descend + addr = h5obj.pathAddresses.get(kp.lstrip("/")) + if addr in seen: + continue # hard-link cycle guard + seen.add(addr) + yield from visit(kp) # kp's children are already in pathAddresses else: yield kp, ds - yield from descend("/") + # Parse the root once to populate its direct children, then descend. + H5Dataset(h5obj, "/", earlyExit=False, metaOnly=True, enableAttributes=False) + yield from visit("/") def full_granule_maps(h5obj, existing: dict[str, ChunkMap]) -> dict[str, ChunkMap]: @@ -340,6 +354,12 @@ def full_granule_maps(h5obj, existing: dict[str, ChunkMap]) -> dict[str, ChunkMa maps[path] = _chunk_map_from_dataset(h5obj, ds, path) except ValueError: continue # COMPACT (or other offset-less layout): not chunk-indexable + except Exception as exc: + # Match the read path's per-dataset tolerance (a bad B-tree or an + # odd object header degrades that dataset to the h5coro decoder + # there): drop just this dataset from the manifest instead of + # sinking the whole granule's write-back. + logger.warning(f" write-back: no chunk map for {path} ({exc}); skipping") return maps @@ -467,17 +487,17 @@ def validate_index_config(cls, index_cfg: dict, data_source: dict | None = None) "index.store is only meaningful for backend 'inline' with " "write_back: true (inline never reads the store)" ) + if "write_back_coverage" in index_cfg and not write_back: + raise ValueError( + "index.write_back_coverage is only meaningful for backend 'inline' " + "with write_back: true" + ) coverage = index_cfg.get("write_back_coverage", "full") if coverage not in cls._COVERAGE: raise ValueError( f"index.write_back_coverage must be one of {sorted(cls._COVERAGE)} " f"(got {coverage!r})" ) - if "write_back_coverage" in index_cfg and not write_back: - raise ValueError( - "index.write_back_coverage is only meaningful for backend 'inline' " - "with write_back: true" - ) # Both read routes accept this backend (issue #170 phase 2): sources # with read_plan.spatial_index take the planned route, read-plan-less # (flat) sources the full-read route -- same compiled addressing seam. diff --git a/tests/test_index.py b/tests/test_index.py index 5eb9176..20633d8 100644 --- a/tests/test_index.py +++ b/tests/test_index.py @@ -1448,6 +1448,29 @@ def test_full_granule_maps_reuses_and_widens(self): # ...and the COMPACT dataset is dropped (no file offset). assert "/ancillary_data/control" not in maps + def test_full_granule_maps_tolerates_one_bad_dataset(self, monkeypatch, caplog): + # A single dataset whose map build raises (not a compact ValueError) + # must not sink the whole granule's write-back -- it degrades like the + # read path (log + skip), and every other dataset is still covered. + import zagg.index.inline as inline_mod + from zagg.index.inline import full_granule_maps + + real = inline_mod._chunk_map_from_dataset + + def flaky(h5obj, ds, path): + if path == "/gt2l/heights/h_ph": + raise RuntimeError("boom") + return real(h5obj, ds, path) + + monkeypatch.setattr(inline_mod, "_chunk_map_from_dataset", flaky) + h5obj = _open_fixture() + with caplog.at_level("WARNING", logger="zagg.index.inline"): + maps = full_granule_maps(h5obj, {}) + assert "/gt2l/heights/h_ph" not in maps # the flaky one dropped + assert "/gt1l/heights/h_ph" in maps # everything else covered + assert "/gt2l/heights/lat_ph" in maps + assert any("boom" in r.message for r in caplog.records) + class TestInlineInterleavedGranules: """Issue #180 phase 1: with granule-level read concurrency the worker may