Avro Table Provider #910

Igosuki · 2021-08-20T17:23:02Z

Which issue does this PR close?

Closes #903.

Rationale for this change

Enables loading avro data files through datafusion.

What changes are included in this PR?

Avro is added as a table provider and a supported file format.
Avro schemas can be translated into arrow schemas.

Are there any user-facing changes?

Yes, as one can now call register_avro on df and 'STORED AS AVRO' in sqsl.

N.B.:

Need to add tests in avro_to_arrow/arrow_array_reader.rs

Missing :

Writing back to avro

I find there is duplication between modules with these additions, I should probably do some refactoring.

alamb · 2021-08-23T20:56:37Z

I plan to start reviewing this PR tomorrow (FYI @houqp in case you want to check it out too). Thank you @Igosuki

Igosuki · 2021-08-24T20:03:00Z

I've added nested field access for pg/generic dialect for things like select array[0][0] in https://github.com/Igosuki/arrow-datafusion/tree/map_access and https://github.com/Igosuki/sqlparser-rs/ based on this branch, which I'm now using in production (although I need to patch ballista to accept these).

Igosuki · 2021-08-24T20:03:43Z

@alamb Thanks

alamb · 2021-08-24T21:55:17Z

Thanks @Igosuki -- I didn't have a chance to review today -- I am hoping to get it tomorrow.

cc @houqp

alamb

Thank you for this contribution @Igosuki. Sorry that took so long. It was quite a bit of code 😅 !

The code is really nicely done, seems very well tested, and was a pleasure to review.

High level things I think we need prior to merging this in:

Make the new avro-rs dependency optional
Commit changes to the arrow-testing repo (or maybe we need a new repo if they decide that having avro files in an arrow repo is not ideal)
Remove the changes for GetFieldExpr (I think we should make that its own PR -- perhaps based off of Added composite identifiers to get field of struct. #628, it just needed someone to push it over the line)
Adding 1 basic end-to-end test to sql.rs that reads one of the newly added avro files and does some simple query on it

I didn't look at the ballista changes carefully -- but I skimmed them and they looked reasonable to me

Also, FWIW I think this PR may be related to #811 (but I would say that PR is still in a bit of a WIP so we shouldn't hold this PR up).

alamb · 2021-08-25T17:39:26Z

.gitmodules

@@ -3,4 +3,5 @@
 	url = https://github.com/apache/parquet-testing.git
 [submodule "testing"]
 	path = testing
-	url = https://github.com/apache/arrow-testing
+	url = https://github.com/Igosuki/arrow-testing.git


prior to merging this PR we should request/merge the changes into apache/arrow-testing I think

This should be good to change back now that the PR is merged.

alamb · 2021-08-25T17:45:48Z

ballista/rust/core/src/serde/logical_plan/to_proto.rs

@@ -1403,6 +1424,9 @@ impl TryInto<protobuf::ScalarFunction> for &BuiltinScalarFunction {
            BuiltinScalarFunction::SHA256 => Ok(protobuf::ScalarFunction::Sha256),
            BuiltinScalarFunction::SHA384 => Ok(protobuf::ScalarFunction::Sha384),
            BuiltinScalarFunction::SHA512 => Ok(protobuf::ScalarFunction::Sha512),
+            BuiltinScalarFunction::ToTimestampMillis => {


the addition of ToTimestampMillis seems unrelated to this PR, but is a good addition

alamb · 2021-08-25T17:51:09Z

datafusion/Cargo.toml

@@ -69,6 +69,8 @@ regex = { version = "^1.4.3", optional = true }
 lazy_static = { version = "^1.4.0", optional = true }
 smallvec = { version = "1.6", features = ["union"] }
 rand = "0.8"
+avro-rs = { version = "0.13", features = ["snappy"] }


As the datafusion codebase already has a non trivial number of depenencies I would personally prefer we did not add additional required ones.

What would you think about making

Suggested change

avro-rs = { version = "0.13", features = ["snappy"] }

avro-rs = { version = "0.13", features = ["snappy"], optional=true }

And then document it as a crate feature -- e.g. like regex_expressions (but not a default feature)?

I think most of the rest of the code in this PR could be left the same and only the part that interacts with avro-rs could be #[cfg(...)] out

alamb · 2021-08-25T17:53:35Z

datafusion/src/logical_plan/dfschema.rs

@@ -140,6 +141,7 @@ impl DFSchema {
                return Ok(i);
            }
        }
+        println!("{}", name);


perhaps a stray debugging leftover

alamb · 2021-08-25T17:55:13Z

datafusion/src/logical_plan/expr.rs

@@ -244,6 +245,13 @@ pub enum Expr {
    IsNull(Box<Expr>),
    /// arithmetic negation of an expression, the operand must be of a signed numeric data type
    Negative(Box<Expr>),
+    /// Returns the field of a [`StructArray`] by name
+    GetField {


This appears to be some part of #628 -- I think it would be cleaner if we revived that PR to get the GetField functionality separately rather than including it in a single large PR

alamb · 2021-08-25T18:28:55Z

datafusion/src/avro_to_arrow/arrow_array_reader.rs

+                    n.into_iter()
+                        .map(|v| {
+                            resolve_string(&v)
+                            // else if matches!(


perhaps this should return an error?

datafusion/src/avro_to_arrow/arrow_array_reader.rs

alamb · 2021-08-25T18:32:44Z

datafusion/src/physical_plan/avro.rs

+                    Ok(Box::pin(AvroStream::new(builder.build(rdr)?, self.limit)))
+                } else {
+                    Err(DataFusionError::Execution(
+                        "Error reading CSV: Data can only be read a single time when the source is a reader"


Suggested change

"Error reading CSV: Data can only be read a single time when the source is a reader"

"Error reading AVRO: Data can only be read a single time when the source is a reader"

alamb · 2021-08-25T18:32:52Z

datafusion/src/physical_plan/avro.rs

+            Source::Reader(rdr) => {
+                if partition != 0 {
+                    Err(DataFusionError::Internal(
+                        "Only partition 0 is valid when CSV comes from a reader"


Suggested change

"Only partition 0 is valid when CSV comes from a reader"

"Only partition 0 is valid when AVRO comes from a reader"

alamb · 2021-08-25T18:34:02Z

datafusion/src/physical_plan/avro.rs

+        Vec::new()
+    }
+
+    fn with_new_children(


This code seems to support creating a table backed by multiple avro files (as is supported by the csv and parquet readers), but I don't see a test for that functionality anywhere.

Maybe you could have a test in sql.rs that refered to the same test files twice and ensured we got back two copies of the data

Added a test for that in sql.rs

Igosuki · 2021-08-25T19:10:13Z

@alamb will address everything soon

Igosuki · 2021-08-26T10:07:05Z

PR in arrow-testing apache/arrow-testing#62

Igosuki · 2021-08-26T13:41:56Z

I think I handled all your comments, let me know if there's more I can do

houqp · 2021-08-29T06:17:30Z

datafusion/Cargo.toml

@@ -69,6 +70,8 @@ regex = { version = "^1.4.3", optional = true }
 lazy_static = { version = "^1.4.0", optional = true }
 smallvec = { version = "1.6", features = ["union"] }
 rand = "0.8"
+avro-rs = { version = "0.13", features = ["snappy"], optional = true }
+num-traits = "0.2"


~~I am not seeing num_traits being used in the code?~~

NVM, I see it's used in the avro reader code :)

It looks like num-traits should be marked as optional and be included as part of avro feature flag?

Suggested change

num-traits = "0.2"

num-traits = { version = "0.2", optional = true }

houqp · 2021-08-29T06:22:27Z

datafusion/src/datasource/mod.rs

@@ -17,6 +17,7 @@

 //! DataFusion data sources

+pub mod avro;


this should be gated by feature flag, no?

Yes, then the feature flags in the avro module can be removed

houqp · 2021-08-29T06:26:06Z

datafusion/src/physical_plan/expressions/nth_value.rs

@@ -78,7 +78,7 @@ impl NthValue {
    }

    /// Create a new NTH_VALUE window aggregate function
-    pub fn nth_value(
+    pub fn value(


maybe first, last and nth would be better names here, cc @jimexist

houqp · 2021-08-29T06:28:40Z

datafusion/Cargo.toml

@@ -69,6 +70,8 @@ regex = { version = "^1.4.3", optional = true }
 lazy_static = { version = "^1.4.0", optional = true }
 smallvec = { version = "1.6", features = ["union"] }
 rand = "0.8"
+avro-rs = { version = "0.13", features = ["snappy"], optional = true }
+num-traits = "0.2"


It looks like num-traits should be marked as optional and be included as part of avro feature flag?

nevi-me · 2021-08-29T09:49:34Z

@Igosuki @houqp @alamb I firstly apologise for not being able to review anything of late :(

I have a question on the avro support, don't know if it's been asked/addresseed yet. Would it be better to move the reader to arrow-rs, so that a writer could also be implemented there?

alamb · 2021-08-29T10:49:36Z

I have a question on the avro support, don't know if it's been asked/addresseed yet. Would it be better to move the reader to arrow-rs, so that a writer could also be implemented there?

@nevi-me I think having a reader in arrow-rs is definitely something to consider (I can definitely see other projects wanting to read from avro files to Arrow arrays). Given this PR is already fairly large I suggest we get it in as is and if there is interest / appetite to moving the avro reader to arrow-rs we do it as a follow on PR -- I filed apache/arrow-rs#727 to track this idea

alamb · 2021-08-29T10:49:51Z

(And welcome back @nevi-me ! )

alamb

I think this is looking great @Igosuki - thank you so much!

I think the needed items prior to merging this are:

Merge Add basic AVRO files (translated copies of the parquet testing files to avro) arrow-testing#62 (review) and update references in this PR
Get a clean CI run (looks like maybe a few more places need #[cfg(feature = "avro")] sprinkled) as well as adding some apache license files

Again, thank you so much

alamb · 2021-08-29T10:54:51Z

datafusion/tests/sql.rs

+            \n    CoalescePartitionsExec\
+            \n      HashAggregateExec: mode=Partial, gby=[], aggr=[COUNT(UInt8(1))]\
+            \n        RepartitionExec: partitioning=RoundRobinBatch(NUM_CORES)\
+            \n          ExecutionPlan(PlaceHolder)\


note the fact that this says Placeholder can be fixed by implementing fmt_as in the Avro ExecutionPlan

alamb · 2021-08-29T10:55:27Z

datafusion/tests/sql.rs

+
+#[cfg(feature = "avro")]
+#[tokio::test]
+async fn avro_explain_analyze() {


I don't think this particular test adds a lot of test coverage -- the non avro version is to ensure execution metrics are propagated correctly.

alamb · 2021-08-29T10:56:20Z

datafusion/tests/sql.rs

+
+#[cfg(feature = "avro")]
+#[tokio::test]
+async fn avro_query_multiple_files() {


alamb · 2021-08-29T10:58:17Z

.github/workflows/rust.yml

@@ -105,13 +105,14 @@ jobs:
        run: |
          export ARROW_TEST_DATA=$(pwd)/testing/data
          export PARQUET_TEST_DATA=$(pwd)/parquet-testing/data
-          # run tests on all workspace members with default feature list
-          cargo test
+          # run tests on all workspace members with default feature list + avro


Igosuki · 2021-08-29T18:53:13Z

Looks like I should have run the entire CI pipeline before committing, will fix

jimexist · 2021-08-30T01:28:18Z

datafusion/src/logical_plan/expr.rs

-                        };
-                        Ok(lit(utf8_val))
+                Expr::Literal(ScalarValue::Utf8(Some(utf8_val))) => {
+                    let utf8_val = if utf8_val == "foo" {


maybe this isn't intended to be merged?

@jimexist I get a clippy failure on nightly :

error: unnecessary nested `if let` or `match` --> datafusion/src/logical_plan/expr.rs:1915:21 | 1915 | / if let ScalarValue::Utf8(Some(utf8_val)) = scalar { 1916 | | let utf8_val = if utf8_val == "foo" { 1917 | | "bar".to_string() 1918 | | } else { ... | 1923 | | Ok(Expr::Literal(scalar)) 1924 | | } | |_____________________^ | = note: `-D clippy::collapsible-match` implied by `-D warnings` help: the outer pattern can be modified to include the inner pattern --> datafusion/src/logical_plan/expr.rs:1914:31 | 1914 | Expr::Literal(scalar) => { | ^^^^^^ replace this binding 1915 | if let ScalarValue::Utf8(Some(utf8_val)) = scalar { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ with this pattern = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_match

Igosuki · 2021-08-30T14:14:29Z

rebased

Igosuki · 2021-09-01T06:15:30Z

Rebased again after new conflict

nevi-me

This looks great. I only reviewed the avro changes in datafusion in detail, so skimmed through ballista.

The feature flag can be simplified before this is merged.

Avro -> Arrow related changes can be addressed when/if moving the code to arrow-rs (which I'd be happy to do)

nevi-me · 2021-09-01T21:40:35Z

datafusion/src/avro_to_arrow/mod.rs

+
+//! This module contains utilities to manipulate avro metadata.
+
+#[cfg(feature = "avro")]


It should be possible to move this feature flag to datafusion/src/lib.rs so you don't include it for each module here

I think this would also be ok to do as a follow on PR

nevi-me · 2021-09-01T21:41:46Z

datafusion/src/avro_to_arrow/mod.rs

+
+#[cfg(feature = "avro")]
+/// Infer Avro schema given a reader
+pub fn infer_avro_schema_from_reader<R: Read + Seek>(reader: &mut R) -> Result<Schema> {


nit: is it inference, or reading the schema from the file/input?

It is actually reading the schema from the input and not infering. I went that way initially, because in the Java world, there is such a use case as infering avro schemas from a stream of avro datums.
The use case would be streaming data into arrow from avro, but it's not the case here so we can simply remove Seek and rename this.

Igosuki@e05ccfc

nevi-me · 2021-09-01T21:46:59Z

datafusion/src/avro_to_arrow/schema.rs

+    name: Option<&str>,
+    nullable: bool,
+) -> Result<Field> {
+    schema_to_field_with_props(schema, name, nullable, Some(&Default::default()))


You could pass None as the props, and then only set the metadata if there are actual properties.

We can adjust this when moving this module to arrow-rs though

nevi-me · 2021-09-01T21:48:40Z

datafusion/src/avro_to_arrow/schema.rs

+        AvroSchema::Decimal {
+            precision, scale, ..
+        } => DataType::Decimal(*precision, *scale),
+        AvroSchema::Uuid => DataType::Utf8,


This should be a binary field, DataType::FixedSizeLen ideally because you otherwise lose the type information (that you're dealing with an UUID).

Extension arrays will also be helpful in future for this. UUID is common enough across different data sources (SQL, Parquet) that we might want to preserve its properties.

Igosuki@94ec6e6

datafusion/src/avro_to_arrow/schema.rs

datafusion/src/datasource/avro.rs

nevi-me · 2021-09-01T21:57:02Z

datafusion/src/datasource/mod.rs

@@ -17,6 +17,7 @@

 //! DataFusion data sources

+pub mod avro;


Yes, then the feature flags in the avro module can be removed

nevi-me · 2021-09-01T22:00:59Z

datafusion/src/physical_plan/datetime_expressions.rs

@@ -236,14 +236,8 @@ pub fn date_trunc(args: &[ColumnarValue]) -> Result<ColumnarValue> {
    let f = |x: Option<i64>| x.map(|x| date_trunc_single(granularity, x)).transpose();

    Ok(match array {
-        ColumnarValue::Scalar(scalar) => {


is this change related to the avro addition?

@nevi-me This is another clippy issue in nightly

nevi-me · 2021-09-01T22:01:41Z

datafusion/src/physical_plan/expressions/nth_value.rs

@@ -78,7 +78,7 @@ impl NthValue {
    }

    /// Create a new NTH_VALUE window aggregate function
-    pub fn nth_value(
+    pub fn value(


If no decision is made here, we could defer the clippy changes until they land on stable

nevi-me · 2021-09-01T22:02:35Z

datafusion/src/physical_plan/mod.rs

@@ -570,6 +570,7 @@ pub trait Accumulator: Send + Sync + Debug {
 pub mod aggregates;
 pub mod analyze;
 pub mod array_expressions;
+pub mod avro;


This should also be feature-flagged so that the flags inside its file can be removed. There's no use compiling the avro module if the flag is disabled.

That implies adding the avro feature flag in a number of other places such as execution/context.rs, is that what you are asking for implicitly ?

I think @nevi-me is suggesting something like

Suggested change

pub mod avro;

#[cfg(feature = "avro")]

pub mod avro;

Which would then let you avoid so much #[cfg...] in #910 (comment)

I am not sure what other changes that entails

@alamb The physical plan is used everywhere in the codebase, so feature gating in the avro submod of physical plan seemed to be the best thing to do

… 'avro'

…om_reader

…rther notice

…test

Dandandan · 2021-09-14T18:03:24Z

It seems adding --release to the workspace tests is a workaround https://github.com/apache/arrow-datafusion/pull/1003/checks?check_run_id=3601789569

(compile time obviously increased by quite a bit)

running 44 tests
test tests::ballista_round_trip::q1 ... ok
test tests::ballista_round_trip::q10 ... ok
test tests::ballista_round_trip::q13 ... ok
test tests::ballista_round_trip::q12 ... ok
test tests::ballista_round_trip::q5 ... ok
test tests::ballista_round_trip::q3 ... ok
test tests::ballista_round_trip::q6 ... ok
test tests::ballista_round_trip::q7 ... ok
test tests::ballista_round_trip::q8 ... ok
test tests::q1 ... ok
test tests::ballista_round_trip::q9 ... ok
test tests::q11 ... ok
test tests::q12 ... ok
test tests::q10 ... ok
test tests::q14 ... ok
test tests::q13 ... ok
test tests::q15 ... ok
test tests::q16 ... ok
test tests::q17 ... ok
test tests::q18 ... ok
test tests::q19 ... ok
test tests::q2 ... ok
test tests::q20 ... ok
test tests::q21 ... ok
test tests::q22 ... ok
test tests::q3 ... ok
test tests::q4 ... ok
test tests::q5 ... ok
test tests::q6 ... ok
test tests::q7 ... ok
test tests::q8 ... ok
test tests::q9 ... ok
test tests::run_q1 ... ok
test tests::run_q10 ... ok
test tests::run_q12 ... ok
test tests::run_q13 ... ok
test tests::run_q14 ... ok
test tests::run_q19 ... ok
test tests::run_q3 ... ok
test tests::run_q6 ... ok
test tests::run_q5 ... ok
test tests::run_q7 ... ok
test tests::run_q8 ... ok
test tests::run_q9 ... ok

test result: ok. 44 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.05s

     Running unittests (/github/home/target/release/deps/ballista-65bbceb93df8237f)

However, those tests now fail somewhere else (extract), because the arrow test data is not available here:

failures:

---- avro_to_arrow::arrow_array_reader::test::test_avro_iterator stdout ----
thread 'avro_to_arrow::arrow_array_reader::test::test_avro_iterator' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', datafusion/src/avro_to_arrow/arrow_array_reader.rs:984:44
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- avro_to_arrow::arrow_array_reader::test::test_avro_read_list stdout ----
thread 'avro_to_arrow::arrow_array_reader::test::test_avro_read_list' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', datafusion/src/avro_to_arrow/arrow_array_reader.rs:984:44

Igosuki · 2021-09-15T11:34:53Z

Tests pass on my local environment setting RUST_MIN_STACK_SIZE without changing cargo, I put it in the github workflow to see if it passes

alamb

Epic work @Igosuki 👍 Thanks again for sticking with this

Igosuki · 2021-09-15T14:39:15Z

Thanks for merging, hope to contribute some more.

Dandandan · 2021-09-15T14:58:54Z

Thanks @Igosuki great contribution! Looking forward to future contributions 😁

jorgecarleitao · 2021-09-15T17:29:44Z

datafusion/src/avro_to_arrow/schema.rs

+        AvroSchema::Enum { symbols, name, .. } => {
+            return Ok(Field::new_dict(
+                &name.fullname(None),
+                index_type(symbols.len()),


Sorry for being late to the party. While porting this to arrow2 I noticed it: I think we want here

DataType::Dictionary(Box::new(index_type(symbols.len())), Box::new(DataType::Utf8))

Field::new_dict does not create a new dict datatype, it is only used to set the dict_id, order, etc.

@jorgecarleitao thanks for pointing that out, I should fix it

github-actions bot added ballista datafusion Changes in the datafusion crate sql SQL Planner labels Aug 20, 2021

Igosuki mentioned this pull request Aug 20, 2021

Avro table provider #903

Closed

Igosuki force-pushed the avro2_m branch from 5272263 to 148a5ab Compare August 24, 2021 13:13

houqp requested review from alamb and Dandandan August 25, 2021 13:49

alamb reviewed Aug 25, 2021

View reviewed changes

houqp reviewed Aug 29, 2021

View reviewed changes

alamb mentioned this pull request Aug 29, 2021

Support for an avro table reader apache/arrow-rs#727

Closed

alamb approved these changes Aug 29, 2021

View reviewed changes

jimexist reviewed Aug 30, 2021

View reviewed changes

Igosuki force-pushed the avro2_m branch from 79f5a07 to d75ddc3 Compare August 30, 2021 14:14

Igosuki force-pushed the avro2_m branch from 77341e4 to 01b055f Compare September 1, 2021 06:15

nevi-me suggested changes Sep 1, 2021

View reviewed changes

houqp requested a review from nevi-me September 4, 2021 06:30

Igosuki added 14 commits September 14, 2021 10:24

Add fmt display implementation for AvroExec

1668b96

ci: fix cargo sql run example, use datafusion/avro feature instead of…

c9218e2

… 'avro'

license: missing license file for avro_to_arrow/schema.rs

be09a64

only run avro datasource tests if features have 'avro'

68df268

refactor: rename infer_avro_schema_from_reader to read_avro_schema_fr…

09c4ecb

…om_reader

Pass None as props to avro schema schema_to_field_with_props until fu…

94b9dcb

…rther notice

Change schema inferance to FixedSizeBinary(16) for Uuid

a03a3c2

schema: prefix metadata coming from avro with 'avro'

405c63a

make num traits optional and part of the avro feature flag

291637a

Fix avro schema tests regarding external props

00f109c

split avro physical plan test feature wise and add a non-implemented …

ea31e02

…test

submodule: switch back to apache/arrow-testing

a264858

fix_test: columns are now prefixed in the plan

c62d931

avro_test: fix clippy warning cmp-owned

aa45189

Igosuki force-pushed the avro2_m branch from cb94c8e to aa45189 Compare September 14, 2021 08:25

avro: move statistics to the physical plan

0b746db

alamb mentioned this pull request Sep 14, 2021

WIP: Test increasing stack size as workaround for stack overflow errors in CI #1001

Closed

Increase min stack size for cargo tests

767c469

alamb approved these changes Sep 15, 2021

View reviewed changes

alamb merged commit 6402200 into apache:master Sep 15, 2021

jorgecarleitao mentioned this pull request Sep 15, 2021

Added support to read Avro. jorgecarleitao/arrow2#406

Merged

jorgecarleitao reviewed Sep 15, 2021

View reviewed changes

alamb mentioned this pull request Sep 15, 2021

Implement Display for Expr, improve operator display #971

Merged

alamb mentioned this pull request Sep 24, 2021

Avoid stack overflow by reducing stack usage of BinaryExpr::evaluate in debug builds #1047

Merged

yjshen mentioned this pull request Sep 27, 2021

Ignore last test, fix cargo clippy, format and pass integration tests houqp/arrow-datafusion#10

Merged

houqp added the enhancement New feature or request label Nov 6, 2021

	avro-rs = { version = "0.13", features = ["snappy"] }
	avro-rs = { version = "0.13", features = ["snappy"], optional=true }

	"Error reading CSV: Data can only be read a single time when the source is a reader"
	"Error reading AVRO: Data can only be read a single time when the source is a reader"

	"Only partition 0 is valid when CSV comes from a reader"
	"Only partition 0 is valid when AVRO comes from a reader"

	num-traits = "0.2"
	num-traits = { version = "0.2", optional = true }


		//! This module contains utilities to manipulate avro metadata.

		#[cfg(feature = "avro")]

Avro Table Provider #910

Avro Table Provider #910

Conversation

Igosuki commented Aug 20, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

N.B.:

Missing :

alamb commented Aug 23, 2021

Igosuki commented Aug 24, 2021

Igosuki commented Aug 24, 2021

alamb commented Aug 24, 2021

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Igosuki commented Aug 25, 2021

Igosuki commented Aug 26, 2021

Igosuki commented Aug 26, 2021

houqp Aug 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevi-me commented Aug 29, 2021

alamb commented Aug 29, 2021

alamb commented Aug 29, 2021

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Igosuki commented Aug 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Igosuki commented Aug 30, 2021

Igosuki commented Sep 1, 2021

nevi-me left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan commented Sep 14, 2021 • edited Loading

Igosuki commented Sep 15, 2021 • edited Loading

alamb left a comment • edited Loading

Choose a reason for hiding this comment

Igosuki commented Sep 15, 2021

Dandandan commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houqp Aug 29, 2021 •

edited

Loading

Dandandan commented Sep 14, 2021 •

edited

Loading

Igosuki commented Sep 15, 2021 •

edited

Loading

alamb left a comment •

edited

Loading