Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: more Results, fewer panics, always have backtraces #761

Merged
merged 54 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 52 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
a9e616d
wip on unwrap_used
lwwmanning Aug 12, 2024
30a5d05
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 12, 2024
e843a33
more pedantic
lwwmanning Aug 12, 2024
dc8f9c4
Use then vs then_some for values that have to be lazy (#599)
robert3005 Aug 12, 2024
938f89a
Vortex physical expressions support for on-disk data (#581)
AdamGS Aug 12, 2024
f57eb3d
wip
lwwmanning Aug 12, 2024
47f6409
wip
lwwmanning Aug 12, 2024
f0ce128
asdfasdf
lwwmanning Aug 12, 2024
b8b0622
add vortex_panic macro
lwwmanning Aug 12, 2024
ee6f42f
Revert "add vortex_panic macro"
lwwmanning Aug 12, 2024
8943ff2
more wip
lwwmanning Aug 12, 2024
b6159f4
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 13, 2024
05b7030
format the world
lwwmanning Aug 13, 2024
e3d4822
fallible_impl_from
lwwmanning Aug 13, 2024
b45bd09
Reapply "add vortex_panic macro"
lwwmanning Aug 13, 2024
8bf9cd3
wip on vortex_panic
lwwmanning Aug 14, 2024
7d3f784
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 14, 2024
d01abd2
remove vortex_panic for now
lwwmanning Aug 14, 2024
e1fac06
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 19, 2024
f147ce7
fixes
lwwmanning Aug 22, 2024
da288b2
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 26, 2024
ab85dac
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 26, 2024
e8300eb
wip
lwwmanning Aug 26, 2024
94daf5f
new vortex_panic macro
lwwmanning Aug 26, 2024
d99f3f5
better
lwwmanning Aug 26, 2024
1ff5ff3
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Aug 28, 2024
5b40bb5
wip
lwwmanning Aug 28, 2024
e89d325
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 3, 2024
87864b3
wip
lwwmanning Sep 4, 2024
e7f3de8
wip:
lwwmanning Sep 4, 2024
128bb46
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 4, 2024
5389d4f
moar
lwwmanning Sep 4, 2024
27437da
wip
lwwmanning Sep 4, 2024
6b504d3
wip
lwwmanning Sep 5, 2024
67e074f
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 5, 2024
8fc215e
moar
lwwmanning Sep 5, 2024
71fb70f
no errors
lwwmanning Sep 6, 2024
03464cf
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 6, 2024
2d93b83
rename byte-bool dir to bytebool for consistency
lwwmanning Sep 6, 2024
48ff8a1
more fixing
lwwmanning Sep 6, 2024
9056ddf
moar
lwwmanning Sep 6, 2024
697fa09
no errors
lwwmanning Sep 6, 2024
97a0b67
format the world
lwwmanning Sep 6, 2024
c8edc66
moar
lwwmanning Sep 6, 2024
8fea5b2
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 6, 2024
3b48fd4
quick self CR
lwwmanning Sep 6, 2024
876cff5
format
lwwmanning Sep 6, 2024
e78a6a8
AssertionFailed for expect failures
lwwmanning Sep 9, 2024
ea157c3
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 9, 2024
e0a7437
self CR
lwwmanning Sep 9, 2024
01cf11b
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 9, 2024
e57e8a7
format
lwwmanning Sep 9, 2024
7e1e926
Merge remote-tracking branch 'origin/develop' into wm/clippy
lwwmanning Sep 10, 2024
d8d5967
fix semantic merge conflict
lwwmanning Sep 10, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 33 additions & 20 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 21 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ uuid = "1.8.0"
vortex-alp = { version = "0.8.0", path = "./encodings/alp" }
vortex-array = { version = "0.8.0", path = "./vortex-array" }
vortex-buffer = { version = "0.8.0", path = "./vortex-buffer" }
vortex-bytebool = { version = "0.8.0", path = "./encodings/byte-bool" }
vortex-bytebool = { version = "0.8.0", path = "./encodings/bytebool" }
vortex-datafusion = { version = "0.8.0", path = "./vortex-datafusion" }
vortex-datetime-dtype = { version = "0.8.0", path = "./vortex-datetime-dtype" }
vortex-datetime-parts = { version = "0.8.0", path = "./encodings/datetime-parts" }
Expand Down Expand Up @@ -171,11 +171,31 @@ unsafe_op_in_unsafe_fn = "deny"

[workspace.lints.clippy]
all = { level = "deny", priority = -1 }
as_ptr_cast_mut = { level = "deny" }
lwwmanning marked this conversation as resolved.
Show resolved Hide resolved
borrow_as_ptr = { level = "deny" }
collection_is_never_read = { level = "deny" }
cognitive_complexity = { level = "deny" }
debug_assert_with_mut_call = { level = "deny" }
derive_partial_eq_without_eq = { level = "deny" }
exit = { level = "deny" }
expect_fun_call = { level = "deny" }
expect_used = { level = "deny" }
equatable_if_let = { level = "deny" }
fallible_impl_from = { level = "deny" }
get_unwrap = { level = "deny" }
host_endian_bytes = { level = "deny" }
if_then_some_else_none = { level = "deny" }
inconsistent_struct_constructor = { level = "deny" }
manual_assert = { level = "deny" }
manual_is_variant_and = { level = "deny" }
mem_forget = { level = "deny" }
or_fun_call = "deny"
panic_in_result_fn = { level = "deny" }
panic = { level = "deny" }
same_name_method = { level = "deny" }
tests_outside_test_module = { level = "deny" }
# todo = { level = "deny" }
# unimplemented = { level = "deny" }
unwrap_in_result = { level = "deny" }
unwrap_used = { level = "deny" }
use_debug = { level = "deny" }
9 changes: 7 additions & 2 deletions bench-vortex/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@ include = { workspace = true }
edition = { workspace = true }
rust-version = { workspace = true }

[lints]
workspace = true
[lints.rust]
warnings = "deny"
unsafe_op_in_unsafe_fn = "deny"

[lints.clippy]
all = { level = "deny", priority = -1 }
or_fun_call = "deny"

[dependencies]
anyhow = { workspace = true }
Expand Down
4 changes: 1 addition & 3 deletions bench-vortex/benches/datafusion_benchmark.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#![allow(clippy::use_debug)]

use std::collections::HashSet;
use std::sync::Arc;

Expand Down Expand Up @@ -83,7 +81,7 @@ fn toy_dataset_arrow() -> RecordBatch {
}

fn toy_dataset_vortex(compress: bool) -> Array {
let uncompressed = toy_dataset_arrow().into();
let uncompressed = toy_dataset_arrow().try_into().unwrap();

if !compress {
return uncompressed;
Expand Down
2 changes: 0 additions & 2 deletions bench-vortex/benches/tpch_benchmark.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#![allow(clippy::use_debug)]

use bench_vortex::tpch::dbgen::{DBGen, DBGenOptions};
use bench_vortex::tpch::{load_datasets, run_tpch_query, tpch_queries, Format};
use criterion::{criterion_group, criterion_main, Criterion};
Expand Down
2 changes: 0 additions & 2 deletions bench-vortex/src/bin/tpch_benchmark.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
#![allow(clippy::use_debug)]

use std::collections::HashMap;
use std::process::ExitCode;
use std::sync;
Expand Down
2 changes: 1 addition & 1 deletion bench-vortex/src/data_downloads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub fn data_vortex_uncompressed(fname_out: &str, downloaded_data: PathBuf) -> Pa
let array = ChunkedArray::try_new(
reader
.into_iter()
.map(|batch_result| Array::from(batch_result.unwrap()))
.map(|batch_result| Array::try_from(batch_result.unwrap()).unwrap())
Copy link
Member

Choose a reason for hiding this comment

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

collect errors and unwrap at the end

.collect(),
dtype,
)
Expand Down
7 changes: 4 additions & 3 deletions bench-vortex/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,8 @@ pub fn compress_taxi_data() -> Array {
let chunks = reader
.into_iter()
.map(|batch_result| batch_result.unwrap())
.map(Array::from)
.map(Array::try_from)
.map(Result::unwrap)
Copy link
Member

Choose a reason for hiding this comment

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

should collect errors and unwrap at the ends

.map(|array| {
uncompressed_size += array.nbytes();
compressor.compress(&array).unwrap()
Expand Down Expand Up @@ -288,7 +289,7 @@ mod test {
let struct_arrow: ArrowStructArray = record_batch.into();
let arrow_array: ArrowArrayRef = Arc::new(struct_arrow);
let vortex_array = Array::from_arrow(arrow_array.clone(), false);
let vortex_as_arrow = vortex_array.into_canonical().unwrap().into_arrow();
let vortex_as_arrow = vortex_array.into_canonical().unwrap().into_arrow().unwrap();
assert_eq!(vortex_as_arrow.deref(), arrow_array.deref());
}
}
Expand All @@ -309,7 +310,7 @@ mod test {
let vortex_array = Array::from_arrow(arrow_array.clone(), false);

let compressed = compressor.compress(&vortex_array).unwrap();
let compressed_as_arrow = compressed.into_canonical().unwrap().into_arrow();
let compressed_as_arrow = compressed.into_canonical().unwrap().into_arrow().unwrap();
assert_eq!(compressed_as_arrow.deref(), arrow_array.deref());
}
}
Expand Down
9 changes: 5 additions & 4 deletions bench-vortex/src/reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use object_store::ObjectStore;
use parquet::arrow::arrow_reader::{ArrowReaderOptions, ParquetRecordBatchReaderBuilder};
use parquet::arrow::async_reader::{AsyncFileReader, ParquetObjectReader};
use parquet::arrow::ParquetRecordBatchStreamBuilder;
use parquet::file::metadata::RowGroupMetaData;
use serde::{Deserialize, Serialize};
use stream::StreamExt;
use vortex::array::{ChunkedArray, PrimitiveArray};
Expand Down Expand Up @@ -58,7 +59,7 @@ pub async fn open_vortex(path: &Path) -> VortexResult<Array> {
.into_array_stream()
.collect_chunked()
.await
.map(|a| a.into_array())
.map(IntoArray::into_array)
}

pub async fn rewrite_parquet_as_vortex<W: VortexWrite>(
Expand Down Expand Up @@ -101,7 +102,7 @@ pub fn compress_parquet_to_vortex(parquet_path: &Path) -> VortexResult<ChunkedAr
let chunks = reader
.map(|batch_result| batch_result.unwrap())
.map(|record_batch| {
let vortex_array = Array::from(record_batch);
let vortex_array = Array::try_from(record_batch).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

should collect errors and unwrap at the end

compressor.compress(&vortex_array).unwrap()
})
.collect_vec();
Expand Down Expand Up @@ -219,7 +220,7 @@ async fn parquet_take_from_stream<T: AsyncFileReader + Unpin + Send + 'static>(
.metadata()
.row_groups()
.iter()
.map(|rg| rg.num_rows())
.map(RowGroupMetaData::num_rows)
.scan(0i64, |acc, x| {
*acc += x;
Some(*acc)
Expand All @@ -238,7 +239,7 @@ async fn parquet_take_from_stream<T: AsyncFileReader + Unpin + Send + 'static>(
let row_group_indices = row_groups
.keys()
.sorted()
.map(|i| row_groups.get(i).unwrap().clone())
.map(|i| row_groups[i].clone())
.collect_vec();

let reader = builder
Expand Down
2 changes: 1 addition & 1 deletion bench-vortex/src/tpch/dbgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ fn get_or_cache_toolchain(
zip_file
.url()
.path_segments()
.and_then(|segments| segments.last())
.and_then(Iterator::last)
.unwrap(),
);

Expand Down
6 changes: 3 additions & 3 deletions bench-vortex/src/tpch/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,8 @@ async fn register_vortex_file(
let sts = record_batches
.iter()
.cloned()
.map(Array::from)
.map(|a| a.into_struct().unwrap())
.map(Array::try_from)
.map(|a| a.unwrap().into_struct().unwrap())
Copy link
Member

Choose a reason for hiding this comment

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

should collect errors

.collect::<Vec<_>>();

let mut arrays_map: HashMap<Arc<str>, Vec<Array>> = HashMap::default();
Expand All @@ -272,7 +272,7 @@ async fn register_vortex_file(
.iter()
.map(|field| {
let name: Arc<str> = field.name().as_str().into();
let dtype = types_map.get(&name).unwrap().clone();
let dtype = types_map[&name].clone();
let chunks = arrays_map.remove(&name).unwrap();

(
Expand Down
1 change: 1 addition & 0 deletions clippy.toml
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
allow-expect-in-tests = true
allow-unwrap-in-tests = true
4 changes: 3 additions & 1 deletion encodings/alp/benches/alp_compress.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![allow(clippy::unwrap_used)]

use arrow::array::{as_primitive_array, ArrowNativeTypeOp, ArrowPrimitiveType};
use arrow::datatypes::{Float32Type, Float64Type};
use divan::{black_box, Bencher};
Expand Down Expand Up @@ -49,7 +51,7 @@ where

fn alp_canonicalize_sum<T: ArrowPrimitiveType>(array: ALPArray) -> T::Native {
let array = array.into_canonical().unwrap().into_arrow();
let arrow_primitive = as_primitive_array::<T>(array.as_ref());
let arrow_primitive = as_primitive_array::<T>(array.as_ref().unwrap());
arrow_primitive
.iter()
.fold(T::default_value(), |acc, value| {
Expand Down
12 changes: 10 additions & 2 deletions encodings/alp/src/alp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::mem::size_of;
use itertools::Itertools;
use num_traits::{Float, NumCast, PrimInt, Zero};
use serde::{Deserialize, Serialize};
use vortex_error::vortex_panic;

const SAMPLE_SIZE: usize = 32;

Expand All @@ -20,7 +21,7 @@ impl Display for Exponents {
}

pub trait ALPFloat: Float + Display + 'static {
type ALPInt: PrimInt;
type ALPInt: PrimInt + Display;

const FRACTIONAL_BITS: u8;
const MAX_EXPONENT: u8;
Expand Down Expand Up @@ -118,7 +119,14 @@ pub trait ALPFloat: Float + Display + 'static {

#[inline]
fn decode_single(encoded: Self::ALPInt, exponents: Exponents) -> Self {
let encoded_float: Self = Self::from(encoded).unwrap();
let encoded_float: Self = Self::from(encoded).unwrap_or_else(|| {
vortex_panic!(
"Failed to convert encoded value {} from {} to {} in ALPFloat::decode_single",
encoded,
std::any::type_name::<Self::ALPInt>(),
std::any::type_name::<Self>()
)
});
encoded_float * Self::F10[exponents.f as usize] * Self::IF10[exponents.e as usize]
}
}
Expand Down
Loading
Loading